Closed Bug 1311044 Opened 3 years ago Closed 3 years ago

non-existent Unix domain socket does not produce an error

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mcs, Assigned: xeonchen)

Details

(Whiteboard: [tor][proxy][necko-active])

Attachments

(2 files)

Attached file stack trace
When a Unix domain socket path is configured as a proxy, the underlying connect() error is not propagated up the networking stack. This causes connections to never fail (and of course they will never succeed). Steps to reproduce:

1. Set the following pref values:
* network.proxy.type = 1
* network.proxy.socks = file:///no-such-file
* network.proxy.socks_port = 0

2. Try to open a page, e.g., https://www.mozilla.org/

Expected results: a page load error should be reported.
Actual results: the page stays in the "Connecting" state indefinitely.

Also, if I use dtruss on OSX I see this repeated rapidly:
connect(0x3A, 0x700000936510, 0x6A)		 = -1 Err#2

(ENOENT is being returned from the connect() system call over and over again).
I will attach a stacktrace.
This bug makes a Unix domain socket proxy unusable for Tor because the firefox process hangs during exit if any network activity is ongoing. Why? Because during browser shutdown the tor proxy daemon is killed, which causes the Unix domain socket to disappear -- but Firefox continues to try to use the non-existent socket.
Assignee: nobody → xeonchen
Whiteboard: [tor][proxy]
Whiteboard: [tor][proxy] → [tor][proxy][necko-active]
Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

https://reviewboard.mozilla.org/r/86856/#review85876

::: netwerk/socket/nsSOCKSIOLayer.cpp:520
(Diff revision 1)
>              PRErrorCode c = PR_GetError();
>              // If EINPROGRESS, return now and check back later after polling
>              if (c == PR_WOULD_BLOCK_ERROR || c == PR_IN_PROGRESS_ERROR) {
>                  mState = SOCKS_CONNECTING_TO_PROXY;
>                  return status;
> +            } else if (c == PR_FILE_NOT_FOUND_ERROR) {

This fix leaves other (less likely?) return codes without action. Wouldn't it be even better to make that an "else" to catch all other errors and treat them the same way?
Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

https://reviewboard.mozilla.org/r/86856/#review85878
Attachment #8802454 - Flags: review?(daniel) → review+
(In reply to Daniel Stenberg [:bagder] from comment #3)
> Comment on attachment 8802454 [details]
> Bug 1311044 - show error when domain socket is not found;
> 
> https://reviewboard.mozilla.org/r/86856/#review85876
> 
> ::: netwerk/socket/nsSOCKSIOLayer.cpp:520
> (Diff revision 1)
> >              PRErrorCode c = PR_GetError();
> >              // If EINPROGRESS, return now and check back later after polling
> >              if (c == PR_WOULD_BLOCK_ERROR || c == PR_IN_PROGRESS_ERROR) {
> >                  mState = SOCKS_CONNECTING_TO_PROXY;
> >                  return status;
> > +            } else if (c == PR_FILE_NOT_FOUND_ERROR) {
> 
> This fix leaves other (less likely?) return codes without action. Wouldn't
> it be even better to make that an "else" to catch all other errors and treat
> them the same way?

I think it just ignores errors and tries next addresses?

See https://dxr.mozilla.org/mozilla-central/rev/01ab78dd98805e150b0311cce2351d5b408f3001/netwerk/socket/nsSOCKSIOLayer.cpp#493
Ah yes, thanks. So other error codes should really continue and try again. Case closed then. Ship it! =)
(In reply to Daniel Stenberg [:bagder] from comment #6)
> Ah yes, thanks. So other error codes should really continue and try again.

GetNextAddr() is not called in the case of a domain socket. I don't think we want to keep trying again for all other errors; that is what led to this bug.
Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

Hi Daniel,

I've just revised the condition checking.
Please see if this makes it better?
Attachment #8802454 - Flags: review+ → feedback?(daniel)
Attachment #8802454 - Flags: feedback?(daniel) → review?(daniel)
Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

https://reviewboard.mozilla.org/r/86856/#review86160

::: netwerk/socket/nsSOCKSIOLayer.cpp:517
(Diff revision 2)
>          NetAddrToPRNetAddr(&proxy, &prProxy);
>          status = fd->lower->methods->connect(fd->lower, &prProxy, mTimeout);
>          if (status != PR_SUCCESS) {
>              PRErrorCode c = PR_GetError();
> +
> +            if (IsHostDomainSocket()) {

This implies that the connect() when using a domain socket can never return a WOULD_BLOCK or IN_PROGRESS error, but I assume that's how it works?
Attachment #8802454 - Flags: review?(daniel) → review+
Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

https://reviewboard.mozilla.org/r/86856/#review86162
Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

https://reviewboard.mozilla.org/r/86856/#review86160

> This implies that the connect() when using a domain socket can never return a WOULD_BLOCK or IN_PROGRESS error, but I assume that's how it works?

Yes, I think so.
Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

https://reviewboard.mozilla.org/r/86856/#review86160

> Yes, I think so.

I've changed my mind, I'll keep those checks.
Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

https://reviewboard.mozilla.org/r/86856/#review86180
That seems safer, I agree.
Pushed by gachen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df49275ec24c
show error when connection to domain socket is failed; r=bagder
https://hg.mozilla.org/mozilla-central/rev/df49275ec24c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.