1344613 - Crash seen in nsSOCKSIOLayer.cpp

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Arthur Edelstein [:arthur]]

While working on Tor Browser (based on ESR52), I ran into a crash in nsSOCKSIOLayer.cpp:
  Program received signal SIGSEGV, Segmentation fault.

Here's part of the stack trace from gdb:

#0  0x00007f71a32ce693 in PR_SetSocketOption (fd=0xe5e5e5e5e5e5e5e5, 
    data=0x7f718ccfc360)
    at /home/arthur/tor-browser/nsprpub/pr/src/io/priometh.c:254
#1  0x00007f7192b3a0e6 in nsSOCKSSocketInfo::ConnectToProxy (
    this=0x7f717ba907d0, fd=0x7f716ccbb3a0)
    at /home/arthur/tor-browser/netwerk/socket/nsSOCKSIOLayer.cpp:582
#2  0x00007f7192b3bc06 in nsSOCKSSocketInfo::DoHandshake (this=0x7f717ba907d0, 
    fd=0x7f716ccbb3a0, oflags=-1)
    at /home/arthur/tor-browser/netwerk/socket/nsSOCKSIOLayer.cpp:1106
#3  0x00007f7192b3c6a5 in nsSOCKSIOLayerConnect (fd=0x7f716ccbb3a0, 
    addr=0x7f718ccfc680, to=20)
    at /home/arthur/tor-browser/netwerk/socket/nsSOCKSIOLayer.cpp:1394
#4  0x00007f71a32cfabd in pl_DefConnect (fd=0x7f716ccbb3d0, 
    addr=0x7f718ccfc680, timeout=20)
    at /home/arthur/tor-browser/nsprpub/pr/src/io/prlayer.c:162
#5  0x00007f71a139a3bf in ssl_SecureConnect (ss=0x7f717ec67000, 
    sa=<optimized out>) at sslsecur.c:729
#6  0x00007f71a139d246 in ssl_Connect (fd=<optimized out>, 
    sockaddr=0x7f718ccfc680, timeout=20) at sslsock.c:2580
#7  0x00007f7195d59e2e in nsSSLIOLayerConnect (fd=0x7f716ccbb220, 
    addr=0x7f718ccfc680, timeout=20)
    at /home/arthur/tor-browser/security/manager/ssl/nsNSSIOLayer.cpp:732
#8  0x00007f71a32ce31f in PR_Connect (fd=0x7f716ccbb220, addr=0x7f718ccfc680, 
    timeout=20) at /home/arthur/tor-browser/nsprpub/pr/src/io/priometh.c:155
#9  0x00007f71928e7970 in mozilla::net::nsSocketTransport::InitiateSocket (
    this=0x7f716ca31000)
    at /home/arthur/tor-browser/netwerk/base/nsSocketTransport2.cpp:1464
#10 0x00007f71928e8bd2 in mozilla::net::nsSocketTransport::OnSocketEvent (
    this=0x7f716ca31000, type=1, status=NS_OK, param=0x0)
    at /home/arthur/tor-browser/netwerk/base/nsSocketTransport2.cpp:1900
#11 0x00007f71929053f9 in mozilla::net::nsSocketEvent::Run (
    this=0x7f716ccbb2e0)
    at /home/arthur/tor-browser/netwerk/base/nsSocketTransport2.cpp:86
#12 0x00007f71927c99b6 in nsThread::ProcessNextEvent (this=0x7f71a1f7e9d0, 
    aMayWait=true, aResult=0x7f718ccfca5f)
    at /home/arthur/tor-browser/xpcom/threads/nsThread.cpp:1216
#13 0x00007f719281b955 in NS_ProcessNextEvent (aThread=0x7f71a1f7e9d0, 
    aMayWait=true) at /home/arthur/tor-browser/xpcom/glue/nsThreadUtils.cpp:361
#14 0x00007f71928eeee4 in mozilla::net::nsSocketTransportService::Run (
    this=0x7f71a1f5dd40)
    at /home/arthur/tor-browser/netwerk/base/nsSocketTransportService2.cpp:939
#15 0x00007f71927c99b6 in nsThread::ProcessNextEvent (this=0x7f71a1f7e9d0, 
    aMayWait=false, aResult=0x7f718ccfccdf)
    at /home/arthur/tor-browser/xpcom/threads/nsThread.cpp:1216

The problem seems to be that the fd argument of PR_SetSocketOption has never been initialized.

Comment 1

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1344613-Prevent-null-pointer-crash-in-nsSOCKSIOL.patch

Here's a fix that initializes nsSOCKSSocketInfo::mFD to nullptr on startup. It gets rid of the crash and seems to behave OK, but I'm not sure if this is the correct solution, though. Gary, does this seem like a good fix, or have I hit an unexpected execution path?

Comment 2

[Gary Chen [:xeonchen]]

(In reply to Arthur Edelstein [:arthuredelstein] from comment #1)
> Created attachment 8843827 [details] [diff] [review]
> 0001-Bug-1344613-Prevent-null-pointer-crash-in-nsSOCKSIOL.patch
> 
> Here's a fix that initializes nsSOCKSSocketInfo::mFD to nullptr on startup.
> It gets rid of the crash and seems to behave OK, but I'm not sure if this is
> the correct solution, though. Gary, does this seem like a good fix, or have
> I hit an unexpected execution path?

Are you using domain socket as your Proxy?

I think you're doing what it supposed to be, thank you.

Comment 3

[Patrick McManus [:mcmanus]]

is this a dup of https://bugzilla.mozilla.org/show_bug.cgi?id=1348841

Comment 4

[Gary Chen [:xeonchen]]

(In reply to Patrick McManus [:mcmanus] from comment #3)
> is this a dup of https://bugzilla.mozilla.org/show_bug.cgi?id=1348841

I'm looking into it, could be related.

Comment 5

[Gary Chen [:xeonchen]]

Sorry I didn't aware and handle it at the first moment, but I think they're duplicated.

Comment 6

[Arthur Edelstein [:arthur]]

Reopening and NEEDINFOing myself because I want to confirm that the null pointer I observed is the same one that was fixed in 1348841.

Comment 7

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1344613-Avoid-possibility-of-null-pointer-crash-.patch

While bug 1348841 does appear to fix the specific crash I saw, I worry it still may be possible that mFD is read before it is initialized. So I would suggest it's still good defensive programming to initialize mFD to nullptr.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 8949944 [details] [diff] [review]
0001-Bug-1344613-Avoid-possibility-of-null-pointer-crash-.patch

Review of attachment 8949944 [details] [diff] [review]:
-----------------------------------------------------------------

thanks

Comment 9

[Arthur Edelstein [:arthur]]

Thanks for the review.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ac867e3e37
Avoid possibility of null pointer crash in nsSOCKSIOLayer.cpp r=mayhemer

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e9ac867e3e37