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

RESOLVED FIXED in mozilla26

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

unspecified
mozilla26
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 892114
(Assignee)

Comment 1

5 years ago
Created attachment 783920 [details] [diff] [review]
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.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 785262 [details] [diff] [review]
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.
(Assignee)

Comment 3

5 years ago
Created attachment 785264 [details] [diff] [review]
Make nsServerSocket::InitWithAddress provide more detailed error results.

Updated for break-out of previous patch; file error codes added; comments improved.
Attachment #783920 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 789440 [details] [diff] [review]
Distinguish PR_ADDRESS_NOT_SUPPORTED_ERROR from other network failures.

Latest version of patch.
Attachment #785262 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 789443 [details] [diff] [review]
Make nsServerSocket::InitWithAddress provide more detailed error results.

Latest version of patch.
Attachment #785264 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #789440 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Attachment #789443 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 6

5 years ago
Created attachment 795476 [details] [diff] [review]
socket-errors.patch

Updated for current mozilla-central.
Attachment #789443 - Attachment is obsolete: true
Attachment #789443 - Flags: review?(jduell.mcbugs)
Attachment #795476 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Attachment #789440 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
(Assignee)

Updated

5 years ago
Attachment #795476 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
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.
Attachment #789440 - Flags: review?(honzab.moz)
Comment on attachment 795476 [details] [diff] [review]
socket-errors.patch

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

8 lines context here as well please.
Attachment #795476 - Flags: review?(honzab.moz)
(Assignee)

Comment 9

5 years ago
Created attachment 798550 [details] [diff] [review]
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.)
Attachment #798550 - Flags: review?(honzab.moz)
(Assignee)

Comment 10

5 years ago
Created attachment 798551 [details] [diff] [review]
Make nsServerSocket::InitWithAddress provide more detailed error results.

Now with eight lines of context.
Attachment #789440 - Attachment is obsolete: true
Attachment #795476 - Attachment is obsolete: true
Attachment #798551 - Flags: review?(honzab.moz)
Attachment #798550 - Flags: review?(honzab.moz) → review+
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.
Attachment #798551 - Flags: review?(ted)
Attachment #798551 - Flags: review?(honzab.moz)
Attachment #798551 - Flags: review+
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.)
Attachment #798551 - Flags: review?(ted) → review+
(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?
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
(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.)
(Assignee)

Comment 17

5 years ago
Pushed the revised patch, just for kicks: https://tbpl.mozilla.org/?tree=Try&rev=4929bfaeb95f
https://hg.mozilla.org/mozilla-central/rev/d9d740e7a3a0
https://hg.mozilla.org/mozilla-central/rev/645e832547c0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.