253190 - PAC: crash on invalid entries

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Joe McCabe]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040726
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040726

My .pac file lists multiple proxies (failover proxies to use if the first is
unavailable).

Mozilla crashes almost immediately after beginning to display content.

Simplifying the .pac so that only a single Proxy is returned makes the crash go
away.

Reproducible: Always
Steps to Reproduce:
1. Use .pac file that looks something like:
 function FindProxyForURL(url, host)
    {return "PROXY 192.168.0.1:3128; 192.168.0.2:3128";} 

(adjust at least the first IP to point to a Proxy in your network)
2. Access a site
3. Restart Mozilla (since it will have just crashed)

Actual Results:  
Crash.

The stack trace ends with:
#0  0x400a63df in PR_AtomicDecrement () from /usr/local/mozilla/libnspr4.so
#1  0x40c29bf3 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#2  0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#3  0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#4  0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#5  0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#6  0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#7  0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#8  0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#9  0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#10 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#11 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#12 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#13 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#14 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#15 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#16 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#17 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#18 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#19 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#20 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so
#21 0x40c29c24 in NSGetModule () from /usr/local/mozilla/components/libnecko.so

Hard to say where the stack began. I followed it as far as:

#73184 0x40c29c24 in NSGetModule ()
   from /usr/local/mozilla/components/libnecko.so
#73185 0x40c29c24 in NSGetModule ()
   from /usr/local/mozilla/components/libnecko.so
#73186 0x40c29c24 in NSGetModule ()
   from /usr/local/mozilla/components/libnecko.so
#73187 0x40c29c24 in NSGetModule ()
   from /usr/local/mozilla/components/libnecko.so
#73188 0x40c29c24 in NSGetModule ()
   from /usr/local/mozilla/components/libnecko.so
#73189 0x40c29c24 in NSGetModule ()
   from /usr/local/mozilla/components/libnecko.so

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

the stack is useless, you created in with a stripped build. can you send a
talkback report, or get a stack using a debug, or at least non-stripped, build?

>#0  0x400a63df in PR_AtomicDecrement () from /usr/local/mozilla/libnspr4.so
>#1  0x40c29bf3 in NSGetModule () from /usr/local/mozilla/components/libnecko.so

*maybe* this means that a component with threadsafe addref/release was released
one time too much...

Comment 2

[Joe McCabe]

Rebuilding with --enable-debug. Will post stack from this build. You have any
other favorite build flags I should use?

Comment 3

[Joe McCabe]

Rebuilt with --enable-debug. The core file isn't much more useful.

#0  0x400c38e2 in PR_AtomicDecrement () from /usr/local/mozilla/libnspr4.so
#1  0x40f1e4f1 in nsCString::operator=(char const*) ()
   from /usr/local/mozilla/components/libnecko.so
#2  0x40f21b41 in nsProxyInfo::~nsProxyInfo() ()
   from /usr/local/mozilla/components/libnecko.so
#3  0x40f1e52c in nsCString::operator=(char const*) ()
   from /usr/local/mozilla/components/libnecko.so
#4  0x40f21b41 in nsProxyInfo::~nsProxyInfo() ()
   from /usr/local/mozilla/components/libnecko.so
#5  0x40f1e52c in nsCString::operator=(char const*) ()
   from /usr/local/mozilla/components/libnecko.so


A new observation of the failure:

The crash does not occur if the .pac file is more strictly compliant with the
format described at
http://wp.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html:

CRASH:
{return "PROXY 192.168.0.1:3128; 192.168.0.2:3128";} 

NO CRASH:
{return "PROXY 192.168.0.1:3128; PROXY 192.168.0.2:3128";} 

Unsure where the bogus syntax came from (I'm having trouble finding other
examples of it).

This bug is likely bogus since the .pac file was incorrectly written. Still -
It'd be nice if the browser didn't crash when Bad Things happen.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

-> me

I could reproduce the bug. I saw an infinite recursion there, with
PR_AtomicDecrement at the top and only nsProxyInfo::Release below...

probably the additional PROXY there confuses the PAC parser and possibly creates
a loop in the nsProxyInfo linked list

Comment 5

[Joe McCabe]

(In reply to comment #4)
> probably the additional PROXY there confuses the PAC parser and possibly creates
> a loop in the nsProxyInfo linked list

Actually it's the lack of the additional PROXY that seems to cause the loop.

CRASH:
{return "PROXY 192.168.0.1:3128; 192.168.0.2:3128";} 

NO CRASH:
{return "PROXY 192.168.0.1:3128; PROXY 192.168.0.2:3128";} 

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

well... *result was never set to null... so it still contained the previous
proxyinfo... -> the linked list had a loop.

Comment 7

[Joe McCabe]

Confirmed that the patch makes the crash go away when a bogus .pac file lists
multiple proxy servers.

You rock

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm, bug 224237 removed this line... and I reviewed that patch...

Comment 9

[Darin Fisher]

Comment on attachment 154502 [details] [diff] [review]
patch

r+sr=darin

thanks biesi!  do we want this on other branches?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

It looks like this broke after 1.7 branched; so, all branches should be fine, I
think.

Checking in netwerk/base/src/nsProtocolProxyService.cpp;
/cvsroot/mozilla/netwerk/base/src/nsProtocolProxyService.cpp,v  <-- 
nsProtocolProxyService.cpp
new revision: 1.57; previous revision: 1.56
done

Comment 11

[benc]

So does the fix support this format, or simply prevent the crash?

Comment 12

[Darin Fisher]

> So does the fix support this format, or simply prevent the crash?

I believe it means that we'd ignore the incorrectly specified server, and
continue parsing after the next ';'

Comment 13

[benc]

Oh, I see what you mean now.