899757 - nsIServerSocket's 'init' and 'initSpecialConnection' should return detailed error codes

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jim Blandy :jimb]

At the moment, nsServerSocket returns NS_ERROR_FAILURE if it is unable to open the socket, bind it to an address, etc. NSPR does return more detailed error codes, which do have better NS_ status codes; nsServerSocket just doesn't bother to look them up.

Comment 1

[Jim Blandy :jimb]

Attachment: Make nsServerSocket::InitWithAddress provide more detailed error results.

I looked through the NSPR socket creation functions that InitWithAddress
uses to see which errors they could return.

- Some NSPR error codes have existing XPCOM error codes that are similar.

- Some do not, and seemed interesting to relay to the developer, so I added
  XPCOM error codes for them. Note that doing so could break code
  specifically testing for NS_ERROR_FAILURE.

- Some seemed too fine-grained or too general to really be useful, so I let
  those continue to return NS_ERROR_FAILURE, with a comment explaining why.

The test coverage is not great; in particular, I wasn't able to find a way
to elicit "address in use" errors from Windows; the web says that Windows
is much more relaxed about binding listening sockets than Unix derivatives.
I'm interested in suggestions.

Comment 2

[Jim Blandy :jimb]

Attachment: Distinguish PR_ADDRESS_NOT_SUPPORTED_ERROR from other network failures.

I broke out this this change on its own, because it seemed to require some care:
PR_ADDRESS_NOT_SUPPORTED_ERROR used to be lumped in with several other NSPR
error codes and reported as NS_ERROR_CONNECTION_REFUSED; and a dumb grep shows
that the NS_ERROR_ is widely checked for. Introducing the distinction might
require the new NS_ERROR_SOCKET_ADDRESS_NOT_SUPPORTED value to be checked for
everywhere that currently checks for NS_ERROR_CONNECTION_REFUSED.

But that seems unlikely to be necessary: first of all, it shouldn't really
be possible, via the XPCOM interface, to force this error path to occur at
present: the components' implementations are in complete control over which
socket address types get used. I'm giving this patch its own try push, with
an assertion in place of the PR_ADDRESS_NOT_SUPPORTED_ERROR case, just to
be sure.

But if that's so, then why introduce the new error code at all? A later
patch adds support for Unix-domain sockets, a type of socket address which
is *not* supported on non-Unix systems. In that case, a distinct error code
will help people diagnose problems quickly.

Comment 3

[Jim Blandy :jimb]

Attachment: Make nsServerSocket::InitWithAddress provide more detailed error results.

Updated for break-out of previous patch; file error codes added; comments improved.

Comment 4

[Jim Blandy :jimb]

Attachment: Distinguish PR_ADDRESS_NOT_SUPPORTED_ERROR from other network failures.

Latest version of patch.

Comment 5

[Jim Blandy :jimb]

Attachment: Make nsServerSocket::InitWithAddress provide more detailed error results.

Latest version of patch.

Comment 6

[Jim Blandy :jimb]

Attachment: socket-errors.patch

Updated for current mozilla-central.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 789440 [details] [diff] [review]
Distinguish PR_ADDRESS_NOT_SUPPORTED_ERROR from other network failures.

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

Jim, submit your patches with context of  lines please.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 795476 [details] [diff] [review]
socket-errors.patch

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

8 lines context here as well please.

Comment 9

[Jim Blandy :jimb]

Attachment: Distinguish PR_ADDRESS_NOT_SUPPORTED_ERROR from other network failures.

Patch now has eight context lines.

(I've fixed my .hgrc; I just forgot to regenerate these patches when re-requesting review. Sorry about that.)

Comment 10

[Jim Blandy :jimb]

Attachment: Make nsServerSocket::InitWithAddress provide more detailed error results.

Now with eight lines of context.

Comment 11

[Jim Blandy :jimb]

Try pushes:
First patch: https://tbpl.mozilla.org/?tree=Try&rev=b3c6ced42861
Second patch: https://tbpl.mozilla.org/?tree=Try&rev=a20d932ab8db

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 798551 [details] [diff] [review]
Make nsServerSocket::InitWithAddress provide more detailed error results.

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

Ted, can you take a quick look at the testing/xpcshell/head.js changes?

::: netwerk/test/unit/test_addr_in_use_error.js
@@ +1,1 @@
> +// Opening a second listening socket on the same address an an extant

an an

@@ +1,3 @@
> +// Opening a second listening socket on the same address an an extant
> +// socket should elicit NS_ERROR_SOCKET_ADDRESS_IN_USE on non-Windows
> +// machines.

Windows has the same behavior.

It is strange that SO_REUSEADDR would not work on linux (which is what you base this test on)

http://hg.mozilla.org/mozilla-central/annotate/e42374f83509/netwerk/base/src/nsServerSocket.cpp#l297


Your test seems to be based on a bug actually :)

@@ +12,5 @@
> +{
> +  // Windows lets us have as many sockets listening on the same address as
> +  // we like, evidently.
> +  if ("@mozilla.org/windows-registry-key;1" in Cc) {
> +    return;

You must do at least one test I think (like 'do_check(true)'), but that could apply only to mochitests.. not sure now.

::: testing/xpcshell/head.js
@@ +870,5 @@
>  
> +// Check that |aFunction| throws an nsIException that has
> +// |Components.results[aResultName]| as the value of its 'result' property.
> +function do_check_throws_nsIException(aFunction, aResultName,
> +                                      stack=Components.stack.caller, todo=false)

I like this, but a peer for the xpcshell should look at this.

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 798551 [details] [diff] [review]
Make nsServerSocket::InitWithAddress provide more detailed error results.

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

The xpcshell harness bits look fine, but could you add a self-test for this to testing/xpcshell/example/unit?

::: testing/xpcshell/head.js
@@ +880,5 @@
> +  }
> +
> +  let msg = ("do_check_throws_nsIException: aFunction should throw" +
> +             " an nsIException whose 'result' is Components.results." +
> +             aResultName);

Why the parens?

@@ +902,5 @@
> +}
> +
> +// Produce a human-readable form of |aException|. This looks up
> +// Components.results values, tries toString methods, and so on.
> +function legibleException(aException)

Naming convention elsewhere in this file is legible_exception. (We also don't seem to use aParam.)

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> Comment on attachment 798551 [details] [diff] [review]
> Make nsServerSocket::InitWithAddress provide more detailed error results.
> 
> Review of attachment 798551 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The xpcshell harness bits look fine, but could you add a self-test for this
> to testing/xpcshell/example/unit?

Thanks Ted.  Would be a followup bug for the self-test bit OK?

Comment 15

[Jim Blandy :jimb]

Thanks for the review!

(In reply to Honza Bambas (:mayhemer) from comment #12)
> an an

Fixed --- thanks.

> @@ +1,3 @@
> > +// Opening a second listening socket on the same address an an extant
> > +// socket should elicit NS_ERROR_SOCKET_ADDRESS_IN_USE on non-Windows
> > +// machines.
> 
> Windows has the same behavior.
> 
> It is strange that SO_REUSEADDR would not work on linux (which is what you
> base this test on)
> 
> http://hg.mozilla.org/mozilla-central/annotate/e42374f83509/netwerk/base/src/
> nsServerSocket.cpp#l297
> 
> 
> Your test seems to be based on a bug actually :)

On Linux, socket(7) says:

SO_REUSEADDR
    Indicates that the rules used in validating addresses supplied in a
    bind(2) call should allow reuse of local addresses. For AF_INET sockets
    this means that a socket may bind, except when there is an active
    listening socket bound to the address. When the listening socket is
    bound to INADDR_ANY with a specific port then it is not possible to
    bind to this port for any local address. Argument is an integer boolean
    flag.

So I think this means that the flag doesn't actually allow sharing addresses once 'listen' has been called. I think it just puts off the error from the 'bind' call until the 'listen' call.

On Windows, however, I think the flag allows sharing of addresses no matter what:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740621%28v=vs.85%29.aspx

    For example, if all of the sockets on the same port provide TCP
    service, any incoming TCP connection requests over the port cannot be
    guaranteed to be handled by the correct socket — the behavior is
    non-deterministic. A malicious program can use SO_REUSEADDR to forcibly
    bind sockets already in use for standard network protocol services in
    order to deny access to those service. No special privileges are
    required to use this option.

So it looks to me as if the test reflects the behavior described above.

> @@ +12,5 @@
> > +{
> > +  // Windows lets us have as many sockets listening on the same address as
> > +  // we like, evidently.
> > +  if ("@mozilla.org/windows-registry-key;1" in Cc) {
> > +    return;
> 
> You must do at least one test I think (like 'do_check(true)'), but that
> could apply only to mochitests.. not sure now.

The try server says that test passes on Windows; I guess xpcshell tests don't have that requirement.

Comment 16

[Jim Blandy :jimb]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> The xpcshell harness bits look fine, but could you add a self-test for this
> to testing/xpcshell/example/unit?

Okay, I added some.

> ::: testing/xpcshell/head.js
> @@ +880,5 @@
> > +  }
> > +
> > +  let msg = ("do_check_throws_nsIException: aFunction should throw" +
> > +             " an nsIException whose 'result' is Components.results." +
> > +             aResultName);
> 
> Why the parens?

They help Emacs indent the second and third lines nicely. If you think they're a distraction I'll take them out.

> @@ +902,5 @@
> > +}
> > +
> > +// Produce a human-readable form of |aException|. This looks up
> > +// Components.results values, tries toString methods, and so on.
> > +function legibleException(aException)
> 
> Naming convention elsewhere in this file is legible_exception. (We also
> don't seem to use aParam.)

Changed. (I hate the aParam convention.)

Comment 17

[Jim Blandy :jimb]

Pushed the revised patch, just for kicks: https://tbpl.mozilla.org/?tree=Try&rev=4929bfaeb95f

Comment 18

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9d740e7a3a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/645e832547c0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9d740e7a3a0
https://hg.mozilla.org/mozilla-central/rev/645e832547c0