For security reasons, we don't want to have the debugging protocol server listening on a TCP port on the phone. Restricting the listener to accept connections only from the local host isn't good enough, as a compromised child process could then use the debugger server to get control of the main process. Unix-domain sockets can be protected using filesystem permissions; child processes run as a different (less-privileged) user, and could thus be prevented from connecting to the debug server. ADB can forward connections to unix-domain sockets, so we can forward debugging connections from the workstation that way.

Those are work-in-progress patches. For a full implementation, there's more to be done: for example, it should be possible to retrieve the address to which an opened socket is bound, but the IDL hasn't been extended to do that now.

What's up with this bug? Coming soon? This is pretty important.

With the patches from bug 899757 applied, the xpcshell test script test_unix_domain.js has been observed to: - create a server Unix-domain socket, - create a client socket connecting to that server socket, - exchange data between client and server, and - check the client socket address. So I guess this is "first light" for this patch.

Comment on attachment 789444 [details] [diff] [review] Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1. Changing "review" request to "feedback"; I'd like to flesh out the tests a bit before opening this up for review; the patch itself has some "to-do" notes as well that I want to take care of.

Okay, this one is ready for review. Try push: https://tbpl.mozilla.org/?tree=Try&rev=04966b9dbf16 (I'm expecting some stray Windows build failures there, and perhaps some OSX issues, but hopefully Linux will be clean...)

Ah, note that WTC landed the NSPR error-reporting improvements in bug 907512, so some of the error handling tests need to be updated (for the better!).

Updated for current sources; tests expanded.

Here's a revision that ensures that nsSocketTransport and nsSocketTransportService properly recognize Unix domain sockets as local to the machine, so they shouldn't be affected by whether nsIIOService is offline or not. Alex, give this one a try.

Here's an interdiff, showing only what I changed last night.

Comment on attachment 796416 [details] [diff] [review] Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1. At the first look, please remove the white-space only changes from the patch please. It makes it hard to review and is no necessary. If you want to remove them, then have a separate patch for it. Thanks.

Whitespace changes and spelling fixes.

Unix-domain sockets, with whitespace changes removed.

Thanks Jim, just reviewing. For info, please use context of 8 lines for the patches next time.

Comment on attachment 797193 [details] [diff] [review] Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1. Review of attachment 797193 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. This looks really good. But I'll rather check on this patch again, quickly this time. I didn't try to run the test, I'll attempt to find time for it, but not block the review on it. ::: netwerk/base/public/nsIServerSocket.idl @@ +145,5 @@ > + * the socket itself is not granted since 2003 (Issue 6). NetBSD says > + * that the permissions on the containing directory (execute) have > + * always applied, so creating sockets in appropriately protected > + * directories should be secure on both old and new systems. > + */ Thanks for this comment! ::: netwerk/base/public/nsISocketTransportService.idl @@ +81,5 @@ > + * @param aPath > + * The file name of the Unix domain socket to which we should > + * connect. > + */ > + nsISocketTransport createUnixDomainTransport(in nsIFile aPath); Change IID of the nsISocketTransportService interface! ::: netwerk/base/src/nsServerSocket.cpp @@ +285,5 @@ > + return rv; > + > + // Create a Unix domain PRNetAddr referring to the given path. > + PRNetAddr addr; > + if (path.Length() + 1 > sizeof(addr.local.path)) So on path.Length() == 0xffffffff we pass this check and copy all data somewhere in memory. Please fix this. *) @@ +288,5 @@ > + PRNetAddr addr; > + if (path.Length() + 1 > sizeof(addr.local.path)) > + return NS_ERROR_FILE_NAME_TOO_LONG; > + addr.local.family = PR_AF_LOCAL; > + memcpy(addr.local.path, path.get(), path.Length()); I'd a little bit more see using BeginReading() to refer the buffer and EndReading() - BeginReading() as a length, but this should work as well. ::: netwerk/base/src/nsSocketTransport2.cpp @@ +865,5 @@ > > nsresult > +nsSocketTransport::InitWithFilename(const char *filename) > +{ > +#if defined(XP_UNIX) || defined(XP_OS2) Does this work on android too? (I presume yes) @@ +868,5 @@ > +{ > +#if defined(XP_UNIX) || defined(XP_OS2) > + size_t filenameLength = strlen(filename); > + > + if (filenameLength + 1 > sizeof(mNetAddr.local.path)) *) @@ +978,5 @@ > if (!mProxyHost.IsEmpty()) { > if (!mProxyTransparent || mProxyTransparentResolvesHost) { > +#if defined(XP_UNIX) || defined(XP_OS2) > + NS_ABORT_IF_FALSE(!mNetAddrIsSet || mNetAddr.raw.family != AF_LOCAL, > + "Unix domain sockets can't be used with proxies"); Indent as it were in if { branch. ::: netwerk/base/src/nsSocketTransportService2.cpp @@ +591,5 @@ > + return NS_ERROR_OUT_OF_MEMORY; > + > + rv = trans->InitWithFilename(path.get()); > + if (NS_FAILED(rv)) > + return rv; Correct indention as per file style (4 spaces)

Comment on attachment 797109 [details] [diff] [review] Whitespace and spelling fixes encountered while working on Unix domain socket support. Review of attachment 797109 [details] [diff] [review]: ----------------------------------------------------------------- r+ but I don't like whitespace only change patches. We should do a global clean up rather as a separate project or just leave it...

Try push: https://tbpl.mozilla.org/?tree=Try&rev=40069d5d894d

Updated per comments. I changed the UUIDs for both the interfaces to which I add methods. >::: netwerk/base/src/nsServerSocket.cpp >> + if (path.Length() + 1 > sizeof(addr.local.path)) > >So on path.Length() == 0xffffffff we pass this check and copy all data >somewhere in memory. Please fix this. True --- but path comes from an nsIFile, which can't be that large. I did a subtraction on the sizeof side instead, in both places. > Does this work on android too? (I presume yes) It would seem to, from looking at configure.ac. Sorry about the indentation mistakes. There's some inconsistency in that directory.

Comment on attachment 798207 [details] [diff] [review] Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1. Review of attachment 798207 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab No time to run the test, I trust you to do it. ::: netwerk/base/src/nsServerSocket.cpp @@ +288,5 @@ > + PRNetAddr addr; > + if (path.Length() > sizeof(addr.local.path) - 1) > + return NS_ERROR_FILE_NAME_TOO_LONG; > + addr.local.family = PR_AF_LOCAL; > + memcpy(addr.local.path, path.BeginReading(), path.Length()); Here you should chose of one of the following approaches, but not mix them: * .BeginReading() to access the buffer, .EndReading() - .BeginReading() to get actual buffer length * .get() to access the buffer, .Length() to get the length It's not for my self entirely clear what is the right way here, so it's up to you what you chose to access the buffer and get the length. To make this 100% clear you may rather ask Boris Zbarsky. ::: netwerk/base/src/nsSocketTransportService2.cpp @@ +589,5 @@ > + nsRefPtr<nsSocketTransport> trans = new nsSocketTransport(); > + if (!trans) > + return NS_ERROR_OUT_OF_MEMORY; > + > + rv = trans->InitWithFilename(path.BeginReading()); Oh, I might not be enough specific. BeginReading() and EndReading() should be used together. When only the buffer is accessed, then .get() should be used. So, here you were OK with .get().

Two hold-ups here: 1) My tests assumed that the notifications that occur when we close down one side of a connection would happen in a particular order, which isn't the case. It's incidental to the test's purpose, so I simplified it, and it should be fine now. 2) We're not able to create the socket at all on Android: TEST-INFO | test_unix_domain.js | "creating socket: /mnt/sdcard/tests/xpcshell/tmp/socket" TEST-UNEXPECTED-FAIL | /mnt/sdcard/tests/xpcshell/head.js | NS_ERROR_CONNECTION_REFUSED: Component returned failure code: 0x804b000d (NS_ERROR_CONNECTION_REFUSED) [nsIServerSocket.initWithFilename] - See following stack: "NS_ERROR_CONNECTION_REFUSED" is socket-speak for "permission denied" (EACCES). The test is calling do_get_tempdir() to get the name of the directory in which to create its sockets; I don't understand why we wouldn't be able to create a socket there.

(In reply to Jim Blandy :jimb from comment #22) > 2) We're not able to create the socket at all on Android: This may be a limitation of the filesystem holding the temp directory on Android; not all filesystems can hold Unix domain sockets. I've disabled the test on Android; if the harness provides a way to give us a suitable path, then we can re-enable it.

Try run looks good. Waiting on review of prerequisite patches in bug 899757.

Grr. I screwed up the xpcshell.ini syntax for disabling test_unix_domain.js on Android, and disabled the whole directory of tests. New try push: https://tbpl.mozilla.org/?tree=Try&rev=52e0448a4bda

(In reply to Jim Blandy :jimb from comment #26) > Grr. I screwed up the xpcshell.ini syntax for disabling test_unix_domain.js > on Android, and disabled the whole directory of tests. New try push: Okay. They all ran this time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/028b937786cb https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a79b1e60b1

https://hg.mozilla.org/mozilla-central/rev/028b937786cb https://hg.mozilla.org/mozilla-central/rev/c0a79b1e60b1