1311044 - non-existent Unix domain socket does not produce an error

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mark Smith [:mcs]]

Attachment: 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.

Comment 1

[Mark Smith [:mcs]]

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.

Comment 2

[Gary Chen [:xeonchen]]

Attachment: Bug 1311044 - show error when connection to domain socket is failed;

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

Comment 3

[--]

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 4

[--]

Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

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

Comment 5

[Gary Chen [:xeonchen]]

(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

Comment 6

[--]

Ah yes, thanks. So other error codes should really continue and try again. Case closed then. Ship it! =)

Comment 7

[Mark Smith [:mcs]]

(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 8

[Gary Chen [:xeonchen]]

Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

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

Comment 9

[Gary Chen [:xeonchen]]

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?

Comment 10

[--]

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?

Comment 11

[--]

Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

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

Comment 12

[Gary Chen [:xeonchen]]

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 13

[Gary Chen [:xeonchen]]

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 14

[Gary Chen [:xeonchen]]

Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

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

Comment 15

[--]

Comment on attachment 8802454 [details]
Bug 1311044 - show error when connection to domain socket is failed;

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

Comment 16

[--]

That seems safer, I agree.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df49275ec24c
show error when connection to domain socket is failed; r=bagder

Comment 18

[Carsten Book [:Tomcat]]

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