1348841 - SOCKS5 broken with Firefox 52 on FreeBSD and perhaps others (regression introduced by bug 1288308)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kai Engert [:KaiE:]]

Let's use this bug to discuss/analyze the regression reported in bug 1288308 comment 71 to 75.

Comment 1

[Kai Engert [:KaiE:]]

Jan, can you please comment, if the suggestion from bug 1288308 (backout) fixes the regression for you?

Comment 2

[Kai Engert [:KaiE:]]

No, my quick-and-dirty backout attempt doesn't build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5234131b5fb598ee59e18249333314e4d541b5c&selectedJob=85047020

Comment 3

[Jan Kokemüller]

When I apply your backout patch on FIREFOX_52_0_RELEASE (and fix the issue with nsNamedPipeIOLayer.h) SOCKS5 works again.

Comment 4

[Jan Kokemüller]

Found the bug: mFD is set to nullptr directly after "ConnectToProxy(mFD)" is called. The latter sets mFD into blocking mode. Then in "HandshakeFinished", mFD is never toggled back into non-blocking mode (because it's null).

This patch to Firefox 52 solves the hangs:


diff --git a/netwerk/socket/nsSOCKSIOLayer.cpp b/netwerk/socket/nsSOCKSIOLayer.cpp
index de82d61..919e581 100644
--- a/netwerk/socket/nsSOCKSIOLayer.cpp
+++ b/netwerk/socket/nsSOCKSIOLayer.cpp
@@ -491,7 +491,6 @@ nsSOCKSSocketInfo::OnLookupComplete(nsICancelable *aRequest,
     mState = SOCKS_DNS_COMPLETE;
     if (mFD) {
       ConnectToProxy(mFD);
-      ForgetFD();
     }
     return NS_OK;
 }

Comment 5

[Kai Engert [:KaiE:]]

Do we have automated SOCKS5 testing? Can this regression be reproduced on other platforms, or FreeBSD, only?

Comment 6

[Kai Engert [:KaiE:]]

Adding reviewers from bug 1288308 to CC.

Gary, are you able to work on a fix for this bug?


(In reply to Jan Kokemüller from comment #4)
> Found the bug: mFD is set to nullptr directly after "ConnectToProxy(mFD)" is
> called. The latter sets mFD into blocking mode. Then in "HandshakeFinished",
> mFD is never toggled back into non-blocking mode (because it's null).

Jan, thanks a lot for finding the cause.

Gary, did you intend to limit "blocking mode during handshake" to Windows?

Should the blocks 
    // Switch to blocking mode during handshaking
and
    // Switch back to nonblocking mode after finishing handshaking.
be limited to Windows?

Is it safe that you are using the same mFD for both the classic fd, and your new named pipe?

Comment 7

[Kai Engert [:KaiE:]]

probably affects all branches ff52 and later.

Comment 9

[Gary Chen [:xeonchen]]

(In reply to Kai Engert (:kaie) from comment #6)
> Gary, did you intend to limit "blocking mode during handshake" to Windows?
> 
> Should the blocks 
>     // Switch to blocking mode during handshaking
> and
>     // Switch back to nonblocking mode after finishing handshaking.
> be limited to Windows?
> 

You're right, that's supposed to be Windows only.

> Is it safe that you are using the same mFD for both the classic fd, and your
> new named pipe?

The mFD for usual case is a temp variable during DNS resolution of proxy's hostname,
it isn't used after |OnLookupComplete|, so I just re-used it in named pipe case rather than
declaring a new one.

Comment 10

[Gary Chen [:xeonchen]]

Attachment: Bug 1348841 - switch on blocking mode for named-pipe only on windows;

Review commit: https://reviewboard.mozilla.org/r/122652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122652/

Comment 11

[Kai Engert [:KaiE:]]

(In reply to Gary Chen [:xeonchen] (use ni? please) from comment #9)
> 
> The mFD for usual case is a temp variable during DNS resolution of proxy's
> hostname,
> it isn't used after |OnLookupComplete|, so I just re-used it in named pipe
> case rather than
> declaring a new one.

It might be more readable, and with less risks for unintentional side effects, if a separate fd member variable is used for the named pipe.

That's just a drive-by comment, I haven't fully read the code, the reviewers should judge if that makes sense, or is unnecessary.

Comment 12

[Daniel Stenberg [:bagder]]

Comment on attachment 8849910 [details]
Bug 1348841 - switch on blocking mode for named-pipe only on windows;

https://reviewboard.mozilla.org/r/122652/#review124904

::: commit-message-20123:1
(Diff revision 1)
> +Bug 1348841 - switch blocking mode (used by named-pipe) only on Windows platform; r=mayhemer,bagder

May I suggest:

"switch on blocking mode for named-pipe only on windows"

::: netwerk/socket/nsSOCKSIOLayer.cpp:429
(Diff revision 1)
>  nsSOCKSSocketInfo::HandshakeFinished(PRErrorCode err)
>  {
>      if (err == 0) {
>          mState = SOCKS_CONNECTED;
> +#if defined(XP_WIN)
>          // Switch back to nonblocking mode after finishing handshaking.

Maybe a brief explanation/comment why this is windows-only?

Comment 13

[Kai Engert [:KaiE:]]

Prior to landing, we should get confirmation that the suggested patch works as a fix.

Jan, would you be able to test and give feedback? Thanks

Comment 14

[Jan Kokemüller]

The patch from attachment #8849910 [details] fixes the SOCKS5 issue for me on FreeBSD. I've tested the patch with current Aurora and Firefox 52.0.1.

Comment 15

[Kai Engert [:KaiE:]]

Gary will you make a new patch to address the review?
Are we waiting for mayhemer's review, or is bagder's review sufficient?
Is the try run (linked from review board) sufficiently green?

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8849910 [details]
Bug 1348841 - switch on blocking mode for named-pipe only on windows;

https://reviewboard.mozilla.org/r/122652/#review125870

r+ based solely on comment 14.  I hope you know what you are doing ;)

Comment 17

[Gary Chen [:xeonchen]]

Comment on attachment 8849910 [details]
Bug 1348841 - switch on blocking mode for named-pipe only on windows;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122652/diff/1-2/

Comment 18

[Gary Chen [:xeonchen]]

Comment on attachment 8849910 [details]
Bug 1348841 - switch on blocking mode for named-pipe only on windows;

https://reviewboard.mozilla.org/r/122652/#review124904

> Maybe a brief explanation/comment why this is windows-only?

I don't really remember the purpose here, and I'm not sure if it's mandatary for Windows.
I'll check this out maybe next Monday when I'm in the office.

Comment 19

[Gary Chen [:xeonchen]]

(In reply to Kai Engert (:kaie) from comment #15)
> Gary will you make a new patch to address the review?
> Are we waiting for mayhemer's review, or is bagder's review sufficient?
> Is the try run (linked from review board) sufficiently green?

I think it's ready to land

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c7f45c7e8a9
switch on blocking mode for named-pipe only on windows; r=bagder,mayhemer

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4c7f45c7e8a9

Comment 22

[Gary Chen [:xeonchen]]

The patch has landed to m-c, Arthur, Jan, would you help to verify if the issue has been fixed in the nightly build?

Patrick, which channels (including ESR52) are you thinking we're going to uplift?
Is it ok to include them all?

Comment 23

[Patrick McManus [:mcmanus]]

I presume this impacts all non-windows platforms (not just freebsd) - please update the title.

its a regression crash fix and should be uplifted (or at least requested) for each impacted release (incl 52 esr)

if for some reason it just impacts bsd (a non tier 1) we could do less.

Comment 24

[Jan Kokemüller]

SOCKS5 proxies work fine again on 350018:d4af7ec6cfcd (FreeBSD).

I did a quick test in a Ubuntu 16.04 VM and could not reproduce the hanging connections with Firefox 52. Maybe some different low level socket behavior masks the issue on Linux?

Comment 25

[Georg Koppen]

Comment on attachment 8849910 [details]
Bug 1348841 - switch on blocking mode for named-pipe only on windows;

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 26

[Georg Koppen]

(In reply to Georg Koppen from comment #25)

Not sure why the usual uplift boílerplate was not shown this time. Here comes the needed information:

> Comment on attachment 8849910 [details]
> Bug 1348841 - switch on blocking mode for named-pipe only on windows;
> 
> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR
> consideration:

The patch fixes potential crashes caused by a patch already landed in mozilla52.

> User impact if declined:

There might be stability issues.

> Fix Landed on Version:

mozilla55

> Risk to taking this patch (and alternatives if risky):

None as far as I can see.
 
> String or UUID changes made by this patch: 

None.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8849910 [details]
Bug 1348841 - switch on blocking mode for named-pipe only on windows;

I think we should uplift this to Aurora/Beta as well for consistency's sake if we intend to uplift this to ESR52 too.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

It doesn't sound like we are sure this affects anything other than FreeBSD (not reproducible on Ubuntu, comment 24)  or have a lot of confidence in the fix (going by comment 14 and other comments)  I'd like to see more certainty before uplift to beta and ESR.

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8849910 [details]
Bug 1348841 - switch on blocking mode for named-pipe only on windows;

We can take this in aurora first and see how it goes. Aurora54+.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/896e9cfb9d67

Comment 31

[Jan Kokemüller]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #28)
> It doesn't sound like we are sure this affects anything other than FreeBSD
> (not reproducible on Ubuntu, comment 24)  or have a lot of confidence in the
> fix (going by comment 14 and other comments)  I'd like to see more certainty
> before uplift to beta and ESR.

While I couldn't reproduce the blocking/nonblocking socket issue on Ubuntu, there is still this crash from bug 1344613, which is related to use of the uninitialized mFD pointer. This is also fixed by this patch and could be relevant for ESR.

This patch puts all changes from bug 1288308 that affected all platforms (not just Windows) behind an "#if defined(XP_WIN)". So after this patch, SOCKS support should be the same again for non-Windows platforms again (as was intended by the Windows-only bug 1288308).

Comment 32

[Dragana Damjanovic [:dragana]]

I am scared when I see NONBLOCKING == false. If it affects bsd it will affect all other non-windows platform. :(

Comment 33

[Georg Koppen]

Do you think the above comments are sufficiently substantiated that they change your mind regarding uplift?

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

OK. Let's go for it. Jan's comment makes sense to me (we should have applied the changes in bug 1288308 to only XP_WIN). And, if leaving this unfixed scares Dragana, that seems conclusive!

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b2ebbc981c90

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/143d5fa98ef7