Unix platforms shouldn't mask errors specific to Unix domain sockets

[Jim Blandy]

... or at least that's the thesis. I'm new to NSPR, so please point me
in the right direction.

The socket manipulation functions in NSPR use errno-to-PR_FOO_ERROR
mapping functions that map errno codes for filesystem-related errors
(EACCESS; ENOENT; etc.) all to PR_ADDRESS_NOT_SUPPORTED. According to
CVS annotate, this code has been present since revision 1.1 (March 1998).

The rationale for this seems to be that "NSPR doesn't support
Unix-domain sockets". However, the rest of NSPR seems to support them
pretty well: PRNetAddr and its associated functions are all prepared for
PR_AF_LOCAL, and connect, etc. seem to work fine. The only missing
pieces I'm aware of are the lack of coverage in pr/tests, and this oddly
preemptive errno code remapping.

(Background: Mozilla's Developer Tools team wants to use Unix domain
sockets as a more secure way to make debugging connections from
development machines to Firefox OS devices. Having the main FxOS process
listen for TCP connections, even only from localhost, is considered too
risky. However, ADB can forward connections to Unix domain sockets, and
the kernel applies filesystem permission checks to such connections,
allowing us to tighten things down a bit more. It would also enable a
more pleasant chrome debugging user experience, as we could place
sockets in profile directories; profile names would be a more meaningful
way to designate debuggees than TCP port numbers.)

I've got patches for Necko, which seem to work pretty well, that add
Unix domain socket support to the necessary XPCOM components. But when I
began testing the error handling, I ran into the strange remappings in
unix_errors.c, and was disappointed.

What would folks say to a patch like the attached?

[Jim Blandy]

On 08/15/2013 05:12 PM, Jim Blandy wrote:
> The socket manipulation functions in NSPR use errno-to-PR_FOO_ERROR
> mapping functions that map errno codes for filesystem-related errors
> (EACCESS; ENOENT; etc.) all to PR_ADDRESS_NOT_SUPPORTED. According to
> CVS annotate, this code has been present since revision 1.1 (March 1998).

Apparently this issue has come up before:

https://bugzilla.mozilla.org/show_bug.cgi?id=270784#c4

WTC wrote: "I think EACCES in this scenario should be mapped to
PR_NO_ACCESS_RIGHTS_ERROR (the default mapping) rather than
PR_ADDRESS_NOT_SUPPORTED_ERROR, but what is done is done."

[Wan-Teh Chang]

On Thu, Aug 15, 2013 at 5:12 PM, Jim Blandy <ji...@mozilla.com> wrote:
>
> What would folks say to a patch like the attached?

Hi Jim,

You didn't attach a patch to your message, but I guess you want to
remove this case from from the _MD_unix_map_connect_error() function
in unix_errors.c, right?

531 case EACCES:
532 prError = PR_ADDRESS_NOT_SUPPORTED_ERROR;
533 break;

In the latest version of the Single Unix Specification, the connect()
man page says EACCES can only be reported in this case:

The connect() function may fail if:

[EACCES]Search permission is denied for a component of the path
prefix; or write access to the named socket is denied.

So it is more correct to map EACCES to PR_NO_ACCESS_RIGHTS_ERROR. Agreed?

Wan-Teh

[Jim Blandy]

Sorry, meant to post this to the newsgroup, not directly to WTC.

-------- Original Message --------
Subject: Re: Unix platforms shouldn't mask errors specific to Unix
domain sockets
Date: Sun, 18 Aug 2013 20:34:58 -0700
From: Jim Blandy <ji...@mozilla.com>
To: Wan-Teh Chang <w...@google.com>

(Huh. The attached patch does show up in the archives here:
https://groups.google.com/d/msg/mozilla.dev.tech.nspr/F57agalOUTw/6J9lHhVDK8UJ

I've included it directly below.)

The patch actually deletes all the switch cases in
_MD_unix_map_connect_error that map errno values to
PR_ADDRESS_NOT_SUPPORTED_ERROR. Deleting those cases has the effect of
delegating those values to _MD_unix_map_default_error, which does map
EACCES to PR_NO_ACCESS_RIGHTS_ERROR, as you suggest.

The errors this affects are those that arise only when using Unix-domain
sockets --- with the exception (that I know of) of EACCES, which Linux
will return for IP and IPv6 addresses if firewall rules forbid the
connection. Even there, I think PR_NO_ACCESS_RIGHTS_ERROR would be the
more informative status to return.

If there are NSPR clients out there that are checking for
PR_ADDRESS_NOT_SUPPORTED_ERROR while using Unix-domain sockets, they're
not going be doing very sophisticated error recovery based on those
codes. If I've thought this through right, then although the patch is
not a backwards-compatible change, it is one that will probably have
little effect beyond improving error messages.

----

Don't mask filesystem-related errors when using Unix domain sockets.
Instead, let _MD_unix_map_default_error handle them normally.

diff --git a/pr/src/md/unix/unix_errors.c b/pr/src/md/unix/unix_errors.c
--- a/pr/src/md/unix/unix_errors.c
+++ b/pr/src/md/unix/unix_errors.c
@@ -528,9 +528,6 @@ void _MD_unix_map_connect_error(int err)
PRErrorCode prError;

switch (err) {
- case EACCES:
- prError = PR_ADDRESS_NOT_SUPPORTED_ERROR;
- break;
#if defined(UNIXWARE)
/*
* On some platforms, if we connect to a port on the local host
@@ -541,12 +538,6 @@ void _MD_unix_map_connect_error(int err)
prError = PR_CONNECT_REFUSED_ERROR;
break;
#endif
- case ELOOP:
- prError = PR_ADDRESS_NOT_SUPPORTED_ERROR;
- break;
- case ENOENT:
- prError = PR_ADDRESS_NOT_SUPPORTED_ERROR;
- break;
case ENXIO:
prError = PR_IO_ERROR;
break;
@@ -565,17 +556,6 @@ void _MD_unix_map_bind_error(int err)
case EINVAL:
prError = PR_SOCKET_ADDRESS_IS_BOUND_ERROR;
break;
- /*
- * UNIX domain sockets are not supported in NSPR
- */
- case EIO:
- case EISDIR:
- case ELOOP:
- case ENOENT:
- case ENOTDIR:
- case EROFS:
- prError = PR_ADDRESS_NOT_SUPPORTED_ERROR;
- break;
default:
_MD_unix_map_default_error(err);
return;

[Wan-Teh Chang]

On Tue, Aug 20, 2013 at 6:09 PM, Jim Blandy <ji...@mozilla.com> wrote:
>
> The patch actually deletes all the switch cases in
> _MD_unix_map_connect_error that map errno values to
> PR_ADDRESS_NOT_SUPPORTED_ERROR. Deleting those cases has the effect of
> delegating those values to _MD_unix_map_default_error, which does map
> EACCES to PR_NO_ACCESS_RIGHTS_ERROR, as you suggest.
>
> The errors this affects are those that arise only when using Unix-domain
> sockets --- with the exception (that I know of) of EACCES, which Linux
> will return for IP and IPv6 addresses if firewall rules forbid the
> connection. Even there, I think PR_NO_ACCESS_RIGHTS_ERROR would be the
> more informative status to return.

Thank you for the patch. I filed an NSPR bug
(https://bugzilla.mozilla.org/show_bug.cgi?id=907512) and checked in
your patch to the NSPR hg repository.

> If there are NSPR clients out there that are checking for
> PR_ADDRESS_NOT_SUPPORTED_ERROR while using Unix-domain sockets, they're
> not going be doing very sophisticated error recovery based on those
> codes. If I've thought this through right, then although the patch is
> not a backwards-compatible change, it is one that will probably have
> little effect beyond improving error messages.

I agree.

Wan-Teh