Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12386010:
Implement IPC::ChannelFactory, a class that accept()s on a UNIX socket. (Closed)
|
Created:
7 years, 9 months ago by jeremya Modified:
7 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, sail+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement IPC::ChannelFactory, a class that accept()s on a UNIX socket.
IPC::ChannelFactory listens on a UNIX domain socket and notifies its delegate
when a client connects. The delegate is expected to craft an IPC::Channel from
the handle it is given.
Previously committed:
- https://src.chromium.org/viewvc/chrome?view=rev&revision=186912
- https://src.chromium.org/viewvc/chrome?view=rev&revision=187233
- https://src.chromium.org/viewvc/chrome?view=rev&revision=187554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187772
Patch Set 1 #Patch Set 2 : Add comments for unix_domain_socket_util #
Total comments: 1
Patch Set 3 : Cleanups, comments, Close() #Patch Set 4 : Use base::FilePath instead of std::string in ChannelFactory's constructor. #
Total comments: 34
Patch Set 5 : Don't close the FD immediately after making it. #Patch Set 6 : comments #
Total comments: 28
Patch Set 7 : Add a test. #Patch Set 8 : comments #Patch Set 9 : GetClientEuid --> GetPeerEuid; verify on client #Patch Set 10 : Don't log ENOENT in pre-bind() unlink(2) #Patch Set 11 : Fix GetPeerEuid usage on client. #
Total comments: 6
Patch Set 12 : comments #
Total comments: 58
Patch Set 13 : comments #
Total comments: 26
Patch Set 14 : comments #Patch Set 15 : fix compile #Patch Set 16 : fix trybots #2 #Patch Set 17 : IPC_EXPORT #Patch Set 18 : fix trybots #3 #Patch Set 19 : fix android trybots #
Total comments: 10
Patch Set 20 : jeremy's comments #Patch Set 21 : rebase #Patch Set 22 : fix ipc_tests on linux_chromeos #Patch Set 23 : rebase #Patch Set 24 : Remove client-side verification #Patch Set 25 : un-revert fix to tests #
Messages
Total messages: 34 (0 generated)
https://codereview.chromium.org/12386010/diff/2001/ipc/ipc.gypi File ipc/ipc.gypi (right): https://codereview.chromium.org/12386010/diff/2001/ipc/ipc.gypi#newcode63 ipc/ipc.gypi:63: 'unix_domain_socket_util.h', I'm not entirely sure how to make sure these only compile on POSIX...
https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:28: LOG(WARNING) << "Unable to create pipe named \"" << path.value() << "\""; Almost every failure case in CreatePipe has already logged something. If you fix the last case that doesn’t log something, this line becomes extraneous. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:39: int local_pipe = -1; Why do you need local_pipe when you can just use &listen_pipe_? https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:44: must_unlink_ = true; Why do yo need a separate variable for this? Isn’t it the same as (listen_pipe_ >= 0)? https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:50: DLOG(INFO) << "Factory creation failed: " << path_.value(); You’ve got lots of LOG(WARNING)s and LOG(ERROR)s, and then you have this case, which is a DLOG (not LOG) and INFO (not something more severe). If you can only reach this after something else that already executed logged something with LOG (not DLOG) and at a higher severity, what’s the point of this added noise? What’s your logging philosophy? I can’t discern any rhyme or reason. Personally, I prefer less logging, and when there is logging, for there to be less of a message (the file and line information is usually more than enough). https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:67: int new_pipe = 0; You probably wanted to initialize this to -1. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:77: DLOG(ERROR) << "Unable to query client euid"; All of the failure cases have already logged something. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:83: // TODO close new pipe Yup, do these TODOs. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:98: DPLOG(ERROR) << "close " << listen_pipe_; Logging the FD number is probably never useful. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:105: } } // namespace IPC https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.h File ipc/ipc_channel_factory.h (right): https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:54: } } // namespace IPC https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:45: PLOG(ERROR) << "close " << pipe_name; Wanna use your FD scoper here too? You’d need to return fd.release(). https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:53: int path_len = snprintf(unix_addr->sun_path, kMaxPipeNameLength, Why not just strncpy? https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:74: file_util::ScopedFD scoped_fd(&fd); …meaning fd will be closed when this function returns, even if it returns success, although you’re giving it back to the caller in server_listen_fd? You’ll want something like *server_listen_fd = fd.release(). https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:86: unix_addr_len) != 0) { Sometimes you write <0 (on line 81), and sometimes != 0 (here and on line 93). Be consistent. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:114: file_util::ScopedFD scoped_fd(&fd); Same thing here. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:171: } // namespace IPC These functions look like they’d be pretty easy to write a couple of simple tests for. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_util.h File ipc/unix_domain_socket_util.h (right): https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.h:13: } } // namespace base https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.h:45: } } // namespace IPC
Tests coming soon to a codereview near you. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:28: LOG(WARNING) << "Unable to create pipe named \"" << path.value() << "\""; On 2013/02/28 04:57:56, Mark Mentovai wrote: > Almost every failure case in CreatePipe has already logged something. If you fix > the last case that doesn’t log something, this line becomes extraneous. Not quite true, I had to add a few extra logs, but I did that and removed this log. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:39: int local_pipe = -1; On 2013/02/28 04:57:56, Mark Mentovai wrote: > Why do you need local_pipe when you can just use &listen_pipe_? Good question. Fixed. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:44: must_unlink_ = true; On 2013/02/28 04:57:56, Mark Mentovai wrote: > Why do yo need a separate variable for this? Isn’t it the same as (listen_pipe_ > >= 0)? Yep, but now that I look at it there's also a case I missed where bind() worked but listen() failed where the socket would stick around. Which I have fixed. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:50: DLOG(INFO) << "Factory creation failed: " << path_.value(); On 2013/02/28 04:57:56, Mark Mentovai wrote: > You’ve got lots of LOG(WARNING)s and LOG(ERROR)s, and then you have this case, > which is a DLOG (not LOG) and INFO (not something more severe). If you can only > reach this after something else that already executed logged something with LOG > (not DLOG) and at a higher severity, what’s the point of this added noise? > > What’s your logging philosophy? I can’t discern any rhyme or reason. ... philosophy? :) A lot of this code was copied in part from other places and then modified. This particular function is analogous to ipc_channel_posix.cc Channel::ChannelImpl::Connect(), which does a DLOG(INFO) in the failure case. The extra log could be helpful when debugging to see what it was that was causing the earlier log messages, but perhaps not useful enough. > Personally, I prefer less logging, and when there is logging, for there to be > less of a message (the file and line information is usually more than enough). Fair enough. I've removed the message. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:67: int new_pipe = 0; On 2013/02/28 04:57:56, Mark Mentovai wrote: > You probably wanted to initialize this to -1. Done. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:77: DLOG(ERROR) << "Unable to query client euid"; On 2013/02/28 04:57:56, Mark Mentovai wrote: > All of the failure cases have already logged something. Removed. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:83: // TODO close new pipe On 2013/02/28 04:57:56, Mark Mentovai wrote: > Yup, do these TODOs. Done. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:98: DPLOG(ERROR) << "close " << listen_pipe_; On 2013/02/28 04:57:56, Mark Mentovai wrote: > Logging the FD number is probably never useful. Done. https://codereview.chromium.org/12386010/diff/3008/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:105: } On 2013/02/28 04:57:56, Mark Mentovai wrote: > } // namespace IPC Done. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:45: PLOG(ERROR) << "close " << pipe_name; On 2013/02/28 04:57:56, Mark Mentovai wrote: > Wanna use your FD scoper here too? You’d need to return fd.release(). Done. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:53: int path_len = snprintf(unix_addr->sun_path, kMaxPipeNameLength, On 2013/02/28 04:57:56, Mark Mentovai wrote: > Why not just strncpy? Done. Do I need to check the return value? strncpy looks pretty straightforward. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:74: file_util::ScopedFD scoped_fd(&fd); On 2013/02/28 04:57:56, Mark Mentovai wrote: > …meaning fd will be closed when this function returns, even if it returns > success, although you’re giving it back to the caller in server_listen_fd? > > You’ll want something like *server_listen_fd = fd.release(). ... yep. good catch, i found this independently after some searching for why it suddenly stopped working. oops. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:86: unix_addr_len) != 0) { On 2013/02/28 04:57:56, Mark Mentovai wrote: > Sometimes you write <0 (on line 81), and sometimes != 0 (here and on line 93). > Be consistent. Done. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:114: file_util::ScopedFD scoped_fd(&fd); On 2013/02/28 04:57:56, Mark Mentovai wrote: > Same thing here. Done. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_util.h File ipc/unix_domain_socket_util.h (right): https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.h:13: } On 2013/02/28 04:57:56, Mark Mentovai wrote: > } // namespace base I thought we only put the closing comment in when the } is far from the {, but I added it anyway. https://codereview.chromium.org/12386010/diff/3008/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.h:45: } On 2013/02/28 04:57:56, Mark Mentovai wrote: > } // namespace IPC Done.
https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:37: if (!CreateServerUnixDomainSocket(path_, &listen_pipe_)) Maybe just return CreateServerUnixDomainSocket(path_, &listen_pipe_); ? https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:72: if (client_euid != geteuid()) { Both clients and servers will use this check on each other, right? https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:83: if (listen_pipe_ >= 0) { I'd sav a level of indentation by doing if (listen_pipe_ < 0) return; https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:84: HANDLE_EINTR(unlink(path_.value().c_str())); Neither the Linux nor the OS X man pages for unlink(2) state that unlink can incur EINTR. I don't think using HANDLE_EINTR is hurting anything, though. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:57: strncpy(unix_addr->sun_path, pipe_name.c_str(), kMaxPipeNameLength); strncpy does not guarantee to NUL-terminate the string! Additionally, you might copy in a truncated path. I think you should enforce that the length of pipe_name is appropriate before doing the strncpy. If it would be truncated or would result in a non-NUL-terminated string, this funtion should fail. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:133: #if defined(OS_MACOSX) || defined(OS_OPENBSD) This method works on FreeBSD >= 4.6 (which is why it works on Mac OS X), FWIW. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:143: return false; Yikes!! Not that Solaris is an officially-supported platform, but do they really not support a way to get the peer's EUID? Oh, I found: http://docs.oracle.com/cd/E19963-01/html/821-1465/getpeerucred-3c.html FWIW. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:144: #else ...and I'm not entirely sure this method *does* work on FreeBSD. So we should use getpeereid on FreeBSD. (It is implemented in terms of LOCAL_PEERCRED, which may or may not be an alias for SO_PEERCRED...)
Still awaiting some tests. https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.h File ipc/ipc_channel_factory.h (right): https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.h... ipc/ipc_channel_factory.h:53: } } // namespace IPC I already mentioned that. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:57: strncpy(unix_addr->sun_path, pipe_name.c_str(), kMaxPipeNameLength); Chris P. wrote: > strncpy does not guarantee to NUL-terminate the string! Additionally, you might > copy in a truncated path. I think you should enforce that the length of > pipe_name is appropriate before doing the strncpy. If it would be truncated or > would result in a non-NUL-terminated string, this funtion should fail. He’s already checked the length on line 33, although it needs to be changed to |>= kMaxPipeNameLength - 1| in order to guarantee that this will be NUL-terminated here. strncpy is fine as long as the length is checked. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:97: const int listen_queue_length = 1; I don’t think you need a separate constant for this here, people who know what listen does know what the argument does. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:168: if (HANDLE_EINTR(close(accept_fd)) < 0) You can use the scoper in this function too.
Test added. Haven't yet responded to chris's comments though On Fri, Mar 1, 2013 at 2:19 PM, <mark@chromium.org> wrote: > Still awaiting some tests. > > > https://codereview.chromium.**org/12386010/diff/10001/ipc/** > ipc_channel_factory.h<https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.h> > File ipc/ipc_channel_factory.h (right): > > https://codereview.chromium.**org/12386010/diff/10001/ipc/** > ipc_channel_factory.h#**newcode53<https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.h#newcode53> > ipc/ipc_channel_factory.h:53: } > } // namespace IPC > > I already mentioned that. > > > https://codereview.chromium.**org/12386010/diff/10001/ipc/** > unix_domain_socket_util.cc<https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_util.cc> > File ipc/unix_domain_socket_util.cc (right): > > https://codereview.chromium.**org/12386010/diff/10001/ipc/** > unix_domain_socket_util.cc#**newcode57<https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_util.cc#newcode57> > ipc/unix_domain_socket_util.**cc:57: strncpy(unix_addr->sun_path, > pipe_name.c_str(), kMaxPipeNameLength); > Chris P. wrote: > >> strncpy does not guarantee to NUL-terminate the string! Additionally, >> > you might > >> copy in a truncated path. I think you should enforce that the length >> > of > >> pipe_name is appropriate before doing the strncpy. If it would be >> > truncated or > >> would result in a non-NUL-terminated string, this funtion should fail. >> > > He’s already checked the length on line 33, although it needs to be > changed to |>= kMaxPipeNameLength - 1| in order to guarantee that this > will be NUL-terminated here. > > strncpy is fine as long as the length is checked. > > https://codereview.chromium.**org/12386010/diff/10001/ipc/** > unix_domain_socket_util.cc#**newcode97<https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_util.cc#newcode97> > ipc/unix_domain_socket_util.**cc:97: const int listen_queue_length = 1; > I don’t think you need a separate constant for this here, people who > know what listen does know what the argument does. > > https://codereview.chromium.**org/12386010/diff/10001/ipc/** > unix_domain_socket_util.cc#**newcode168<https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_util.cc#newcode168> > ipc/unix_domain_socket_util.**cc:168: if (HANDLE_EINTR(close(accept_fd)**) > < > 0) > You can use the scoper in this function too. > > https://codereview.chromium.**org/12386010/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:37: if (!CreateServerUnixDomainSocket(path_, &listen_pipe_)) On 2013/02/28 23:48:28, Chris P. wrote: > Maybe just > > return CreateServerUnixDomainSocket(path_, &listen_pipe_); > > ? lol, good catch. https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:72: if (client_euid != geteuid()) { On 2013/02/28 23:48:28, Chris P. wrote: > Both clients and servers will use this check on each other, right? Currently only the server checks. https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:83: if (listen_pipe_ >= 0) { On 2013/02/28 23:48:28, Chris P. wrote: > I'd sav a level of indentation by doing > > if (listen_pipe_ < 0) > return; Done. https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:83: if (listen_pipe_ >= 0) { On 2013/02/28 23:48:28, Chris P. wrote: > I'd sav a level of indentation by doing > > if (listen_pipe_ < 0) > return; Done. https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:84: HANDLE_EINTR(unlink(path_.value().c_str())); On 2013/02/28 23:48:28, Chris P. wrote: > Neither the Linux nor the OS X man pages for unlink(2) state that unlink can > incur EINTR. > > I don't think using HANDLE_EINTR is hurting anything, though. Cool, I was confused when I should use it and when not. Removed. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:57: strncpy(unix_addr->sun_path, pipe_name.c_str(), kMaxPipeNameLength); On 2013/03/01 03:19:17, Mark Mentovai wrote: > Chris P. wrote: > > strncpy does not guarantee to NUL-terminate the string! Additionally, you > might > > copy in a truncated path. I think you should enforce that the length of > > pipe_name is appropriate before doing the strncpy. If it would be truncated or > > would result in a non-NUL-terminated string, this funtion should fail. > > He’s already checked the length on line 33, although it needs to be changed to > |>= kMaxPipeNameLength - 1| in order to guarantee that this will be > NUL-terminated here. > > strncpy is fine as long as the length is checked. I don't think it needs to be NUL-terminated, as the addr structure has a path length in it. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:97: const int listen_queue_length = 1; On 2013/03/01 03:19:17, Mark Mentovai wrote: > I don’t think you need a separate constant for this here, people who know what > listen does know what the argument does. Done. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:133: #if defined(OS_MACOSX) || defined(OS_OPENBSD) On 2013/02/28 23:48:28, Chris P. wrote: > This method works on FreeBSD >= 4.6 (which is why it works on Mac OS X), FWIW. Not sure what you're implying here. Should I remove or add a #if somewhere? https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:143: return false; On 2013/02/28 23:48:28, Chris P. wrote: > Yikes!! Not that Solaris is an officially-supported platform, but do they really > not support a way to get the peer's EUID? Oh, I found: > > http://docs.oracle.com/cd/E19963-01/html/821-1465/getpeerucred-3c.html > > FWIW. Just copied from the ipc_channel_posix.cc implementation of this. I've removed the OS_SOLARIS bit, seems silly to keep it. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:144: #else On 2013/02/28 23:48:28, Chris P. wrote: > ...and I'm not entirely sure this method *does* work on FreeBSD. So we should > use getpeereid on FreeBSD. (It is implemented in terms of LOCAL_PEERCRED, which > may or may not be an alias for SO_PEERCRED...) Eh? What about Linux? https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:168: if (HANDLE_EINTR(close(accept_fd)) < 0) On 2013/03/01 03:19:17, Mark Mentovai wrote: > You can use the scoper in this function too. Done.
https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:72: if (client_euid != geteuid()) { > Currently only the server checks. Ahh. We'll need for both to check. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:133: #if defined(OS_MACOSX) || defined(OS_OPENBSD) > Not sure what you're implying here. Should I remove or add a #if somewhere? I'd say: #if defined(OS_MACOSX) || defined(OS_OPENBSD) || defined(OS_FREEBSD) Currently, FreeBSD falls through to the #else, and I'm not sure that code works on FreeBSD. (Haven't checked though.) https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:144: #else > Eh? What about Linux? This #else block works/should work fine as-is for Linux.
https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:72: if (client_euid != geteuid()) { On 2013/03/01 19:14:30, Chris P. wrote: > > Currently only the server checks. > > Ahh. We'll need for both to check. Done. https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/10001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:133: #if defined(OS_MACOSX) || defined(OS_OPENBSD) On 2013/03/01 19:14:30, Chris P. wrote: > > Not sure what you're implying here. Should I remove or add a #if somewhere? > > I'd say: > > #if defined(OS_MACOSX) || defined(OS_OPENBSD) || defined(OS_FREEBSD) > > Currently, FreeBSD falls through to the #else, and I'm not sure that code works > on FreeBSD. (Haven't checked though.) Done.
Please fix the unlink/PLOG thing before committing, but LGTM modulo two minor documentation nits. https://codereview.chromium.org/12386010/diff/11003/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/11003/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:83: unlink(path_.value().c_str()); I'd still check the return value and PLOG if it fails. https://codereview.chromium.org/12386010/diff/11003/ipc/ipc_channel_factory.h File ipc/ipc_channel_factory.h (right): https://codereview.chromium.org/12386010/diff/11003/ipc/ipc_channel_factory.h... ipc/ipc_channel_factory.h:14: // A ChannelFactory listens on a UNIX domain socket and passes new FDs to its NIT: Clarify where the new FDs come from? https://codereview.chromium.org/12386010/diff/11003/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.h (right): https://codereview.chromium.org/12386010/diff/11003/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.h:23: // Creates a UNIX-domain socket at |pipe_name| and connect()s to it. If NIT: I'd say "opens" here instead of "creates".
https://codereview.chromium.org/12386010/diff/11003/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/11003/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:83: unlink(path_.value().c_str()); On 2013/03/05 00:12:31, Chris P. wrote: > I'd still check the return value and PLOG if it fails. Done, plus I moved the unlink() after the close(). https://codereview.chromium.org/12386010/diff/11003/ipc/ipc_channel_factory.h File ipc/ipc_channel_factory.h (right): https://codereview.chromium.org/12386010/diff/11003/ipc/ipc_channel_factory.h... ipc/ipc_channel_factory.h:14: // A ChannelFactory listens on a UNIX domain socket and passes new FDs to its On 2013/03/05 00:12:31, Chris P. wrote: > NIT: Clarify where the new FDs come from? Done, does the new comment make sense? https://codereview.chromium.org/12386010/diff/11003/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.h (right): https://codereview.chromium.org/12386010/diff/11003/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.h:23: // Creates a UNIX-domain socket at |pipe_name| and connect()s to it. If On 2013/03/05 00:12:31, Chris P. wrote: > NIT: I'd say "opens" here instead of "creates". Done.
https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:81: if (!file_util::CreateDirectory(pipe_dir)) { Hm, problem: IPC channels normally want to be used on the IO thread, but the IO thread disallows IO (ha), so this has to be called on the FILE thread. Should this use a base::ThreadRestricitons::ScopedAllowIO?
Don’t let the volume of comments disappoint you, that’s more a factor of the size of the change, and almost all of the comments are very minor. The ECONNABORTED thing is the only major thing here, and that should be easy to address. I expect that this will be LG on the next round. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel.h File ipc/ipc_channel.h (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel.h#newcode170 ipc/ipc_channel.h:170: // which case the supplied client_euid is updated with it. client_euid → peer_euid now. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:7: #include <errno.h> I’m surprised to see this many #includes for a file that’s so short. For example, I don’t see any uses of errno or the error constants in this file, but here <errno.h> is. I don’t see any constants, types, or functions from fcntl.h, but here’s <fcntl.h> (close doesn’t count, it comes from <unistd.h>). Can you clean this up? https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:43: // Watch the pipe for connections, and turn any connections into I like to see a blank line before comments like this one (that aren’t at the beginning of a new {scope}), because otherwise the comment tends to visually blend in with the surrounding code. Same on line 64. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:59: Close(); I’m not sure that Close() is appropriate here. What if accept() returns ECONNABORTED? This might happen if a client attempts to connect and then gives up before you have a chance to call accept(). You don’t want to just throw the whole server away when that happens, right? https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:67: close(new_pipe); Close does HANDLE_EINTR around its close. This close and the one on line 72 are bare. Be consistent. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h File ipc/ipc_channel_factory.h (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:41: bool CreatePipe(); This “pipe” business is a little bit misleading. I think of “pipe” as a “named pipe,” where you’re dealing with “UNIX-domain sockets” which are similar but different. Is there a better name that you could use for CreatePipe(), listen_pipe_, and some of the comments and local variables in the .cc file? https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:43: virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {} Inlining this (empty) function here seems like more trouble than it’s worth. You can move it to the .cc file and you can also stick a NOTREACHED() in it. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:53: } } // namespace IPC https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_posix.cc#n... ipc/ipc_channel_posix.cc:267: if (!IPC::GetPeerEuid(local_pipe, &server_euid)) { You have identical logic in ipc_channel_factory.cc. Can you factor those out into a common routine, like IPC::IsPeerAuthorized(int fd)? https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:27: int MakeUnixAddrForPath(const std::string& pipe_name, The comment should explain these parameters, because it’s not clear just from reading this prototype whether unix_addr and unix_addr_len are in/out-parameters or just out-parameters. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:27: int MakeUnixAddrForPath(const std::string& pipe_name, Similar comments about the use of the name “pipe” in this file apply as well. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:33: if (pipe_name.length() == 0 || pipe_name.length() >= kMaxPipeNameLength) { I know you said that you didn’t think you needed to worry about pipe_name.length == kMaxPipeNameLength because the sockaddr_un structure contains the sun_len field which encodes the length distinctly from the sun_path field, thus rendering NUL-termination unnecessary. I don’t think that this is correct. Some code expects sockaddr_un::sun_path to be NUL-terminated. See, for example, the definition of the SUN_LEN macro in <sys/un.h> on both Mac and Linux. See “man 7 unix” on Linux as well. Anyway, this looks correct now. I just wanted to make sure that we all agree that it’s correct for the right reasons, and not by accident. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:34: LOG(ERROR) << "Pipe name too long: " << pipe_name; Or short. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:43: } You should make a scoped_fd here (and return scoper.release()) so you can do away with the manual close() on line 48 and make this code more friendly to future modifications that might want to add early returns. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:47: PLOG(ERROR) << "fcntl(O_NONBLOCK) " << pipe_name; pipe_name is meaningless for error reporting for any system call on a socket FD before the socket has been bound. Same on line 49. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:60: offsetof(struct sockaddr_un, sun_path) + pipe_name.length() + 1; I think that the +1 isn’t necessary. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:81: if (!file_util::CreateDirectory(pipe_dir)) { jeremya wrote: > Hm, problem: IPC channels normally want to be used on the IO thread, but the IO > thread disallows IO (ha), so this has to be called on the FILE thread. Should > this use a base::ThreadRestricitons::ScopedAllowIO? Oh, this sucks! The problem is actually more than just this call: the unlink and bind also go out to the filesystem. No great answer for you here. Allowing IO sucks. Just mandating that this directory exist when you’re called could work, but the bind() will still do filesystem IO, so it would get you past Chrome’s checks (since we don’t wrap bind() in something that checks whether IO is allowed or whether IO will be done) but wouldn’t really get you past the problem of doing IO on this thread. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:87: if (unlink(pipe_name.c_str()) < 0 && errno != ENOENT) This is where you want to #include <errno.h>. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:98: if (listen(fd, 1) < 0) { 1 is small. Use SOMAXCONN? https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:163: return false; “false” may be a little simplistic as a return value. Consider my previous comment about ECONNABORTED. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... File ipc/unix_domain_socket_util_unittest.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:32: int server_fd() { return server_fd_; } This method can be const, just for kicks. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:89: // TODO stat socket_name and check that there's a socket present OK, so do this. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:146: size_t buf_len = strlen(buffer) + 1; You could just use sizeof(buffer). https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:147: size_t sent_bytes = send(connection.client_fd(), buffer, buf_len, 0); HANDLE_EINTR here and on line 151. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:149: scoped_array<char> recv_buf(new char[buf_len]); You could just have a char recv_buf[sizeof(buffer)] instead of having to deal with new and a scoper. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:153: ASSERT_EQ(0, strncmp(recv_buf.get(), buffer, buf_len)); Use memcmp. You have sizes in hand and you want to treat embedded NULs transparently. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:156: } } // namespace
https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h File ipc/ipc_channel_factory.h (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:15: // created when a client connects to its delegate. It is the delegate's "FDs that get created" — by whom? Passes the FDs to where? "to its delegate" could also apply to "when a client connects". :)
https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel.h File ipc/ipc_channel.h (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel.h#newcode170 ipc/ipc_channel.h:170: // which case the supplied client_euid is updated with it. On 2013/03/05 20:08:03, Mark Mentovai wrote: > client_euid → peer_euid now. I see your unicode arrow and I raise you U+104D ၍ MYANMAR SYMBOL COMPLETED https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:7: #include <errno.h> On 2013/03/05 20:08:03, Mark Mentovai wrote: > I’m surprised to see this many #includes for a file that’s so short. For > example, I don’t see any uses of errno or the error constants in this file, but > here <errno.h> is. I don’t see any constants, types, or functions from fcntl.h, > but here’s <fcntl.h> (close doesn’t count, it comes from <unistd.h>). Can you > clean this up? Yeah, I think I must have just copied this in along with the unix domain socket stuff before I factored it out. Turns out none of these <headers> were needed. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:43: // Watch the pipe for connections, and turn any connections into On 2013/03/05 20:08:03, Mark Mentovai wrote: > I like to see a blank line before comments like this one (that aren’t at the > beginning of a new {scope}), because otherwise the comment tends to visually > blend in with the surrounding code. Same on line 64. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:59: Close(); On 2013/03/05 20:08:03, Mark Mentovai wrote: > I’m not sure that Close() is appropriate here. What if accept() returns > ECONNABORTED? This might happen if a client attempts to connect and then gives > up before you have a chance to call accept(). You don’t want to just throw the > whole server away when that happens, right? Yeah, you're right. I've changed the logic so that ServerAcceptConnection() only returns false if an unrecoverable error occurs, and this function takes the signal new_pipe >= 0 to indicate that a socket was successfully opened. Where ipc_channel_posix uses this I just check for both failure conditions. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.cc... ipc/ipc_channel_factory.cc:67: close(new_pipe); On 2013/03/05 20:08:03, Mark Mentovai wrote: > Close does HANDLE_EINTR around its close. This close and the one on line 72 are > bare. Be consistent. file_util::ScopedFD to the rescue. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h File ipc/ipc_channel_factory.h (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:15: // created when a client connects to its delegate. It is the delegate's On 2013/03/06 00:59:21, Chris P. wrote: > "FDs that get created" — by whom? > > Passes the FDs to where? "to its delegate" could also apply to "when a client > connects". :) Updated the comment :) https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:41: bool CreatePipe(); On 2013/03/05 20:08:03, Mark Mentovai wrote: > This “pipe” business is a little bit misleading. I think of “pipe” as a “named > pipe,” where you’re dealing with “UNIX-domain sockets” which are similar but > different. Is there a better name that you could use for CreatePipe(), > listen_pipe_, and some of the comments and local variables in the .cc file? Done. s/pipe/fd/g, s/CreatePipe/CreateSocket https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:43: virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE {} On 2013/03/05 20:08:03, Mark Mentovai wrote: > Inlining this (empty) function here seems like more trouble than it’s worth. > > You can move it to the .cc file and you can also stick a NOTREACHED() in it. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_factory.h#... ipc/ipc_channel_factory.h:53: } On 2013/03/05 20:08:03, Mark Mentovai wrote: > } // namespace IPC Done. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel_posix.cc#n... ipc/ipc_channel_posix.cc:267: if (!IPC::GetPeerEuid(local_pipe, &server_euid)) { On 2013/03/05 20:08:03, Mark Mentovai wrote: > You have identical logic in ipc_channel_factory.cc. Can you factor those out > into a common routine, like IPC::IsPeerAuthorized(int fd)? Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:27: int MakeUnixAddrForPath(const std::string& pipe_name, On 2013/03/05 20:08:03, Mark Mentovai wrote: > The comment should explain these parameters, because it’s not clear just from > reading this prototype whether unix_addr and unix_addr_len are in/out-parameters > or just out-parameters. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:27: int MakeUnixAddrForPath(const std::string& pipe_name, On 2013/03/05 20:08:03, Mark Mentovai wrote: > Similar comments about the use of the name “pipe” in this file apply as well. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:33: if (pipe_name.length() == 0 || pipe_name.length() >= kMaxPipeNameLength) { On 2013/03/05 20:08:03, Mark Mentovai wrote: > I know you said that you didn’t think you needed to worry about pipe_name.length > == kMaxPipeNameLength because the sockaddr_un structure contains the sun_len > field which encodes the length distinctly from the sun_path field, thus > rendering NUL-termination unnecessary. I don’t think that this is correct. Some > code expects sockaddr_un::sun_path to be NUL-terminated. See, for example, the > definition of the SUN_LEN macro in <sys/un.h> on both Mac and Linux. See “man 7 > unix” on Linux as well. > > Anyway, this looks correct now. I just wanted to make sure that we all agree > that it’s correct for the right reasons, and not by accident. I see. How confusing. I added a comment here to clarify lest someone later see the same logic that I did. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:34: LOG(ERROR) << "Pipe name too long: " << pipe_name; On 2013/03/05 20:08:03, Mark Mentovai wrote: > Or short. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:43: } On 2013/03/05 20:08:03, Mark Mentovai wrote: > You should make a scoped_fd here (and return scoper.release()) so you can do > away with the manual close() on line 48 and make this code more friendly to > future modifications that might want to add early returns. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:47: PLOG(ERROR) << "fcntl(O_NONBLOCK) " << pipe_name; On 2013/03/05 20:08:03, Mark Mentovai wrote: > pipe_name is meaningless for error reporting for any system call on a socket FD > before the socket has been bound. Same on line 49. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:60: offsetof(struct sockaddr_un, sun_path) + pipe_name.length() + 1; On 2013/03/05 20:08:03, Mark Mentovai wrote: > I think that the +1 isn’t necessary. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:81: if (!file_util::CreateDirectory(pipe_dir)) { On 2013/03/05 20:08:03, Mark Mentovai wrote: > jeremya wrote: > > Hm, problem: IPC channels normally want to be used on the IO thread, but the > IO > > thread disallows IO (ha), so this has to be called on the FILE thread. Should > > this use a base::ThreadRestricitons::ScopedAllowIO? > > Oh, this sucks! > > The problem is actually more than just this call: the unlink and bind also go > out to the filesystem. > > No great answer for you here. Allowing IO sucks. Just mandating that this > directory exist when you’re called could work, but the bind() will still do > filesystem IO, so it would get you past Chrome’s checks (since we don’t wrap > bind() in something that checks whether IO is allowed or whether IO will be > done) but wouldn’t really get you past the problem of doing IO on this thread. Allowing IO might not be so bad as this is a rare operation (~1 per Chrome session). It's certainly easier than posting a task over to the file thread and forcing channel creation to be asynchronous (it looks like certain things assume/require that channel creation happen synchronously). I wouldn't be super worried about allowing IO here. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:87: if (unlink(pipe_name.c_str()) < 0 && errno != ENOENT) On 2013/03/05 20:08:03, Mark Mentovai wrote: > This is where you want to #include <errno.h>. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:98: if (listen(fd, 1) < 0) { On 2013/03/05 20:08:03, Mark Mentovai wrote: > 1 is small. Use SOMAXCONN? Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util.cc:163: return false; On 2013/03/05 20:08:03, Mark Mentovai wrote: > “false” may be a little simplistic as a return value. Consider my previous > comment about ECONNABORTED. Addressed in a previous reply. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... File ipc/unix_domain_socket_util_unittest.cc (right): https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:32: int server_fd() { return server_fd_; } On 2013/03/05 20:08:03, Mark Mentovai wrote: > This method can be const, just for kicks. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:89: // TODO stat socket_name and check that there's a socket present On 2013/03/05 20:08:03, Mark Mentovai wrote: > OK, so do this. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:146: size_t buf_len = strlen(buffer) + 1; On 2013/03/05 20:08:03, Mark Mentovai wrote: > You could just use sizeof(buffer). Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:147: size_t sent_bytes = send(connection.client_fd(), buffer, buf_len, 0); On 2013/03/05 20:08:03, Mark Mentovai wrote: > HANDLE_EINTR here and on line 151. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:149: scoped_array<char> recv_buf(new char[buf_len]); On 2013/03/05 20:08:03, Mark Mentovai wrote: > You could just have a char recv_buf[sizeof(buffer)] instead of having to deal > with new and a scoper. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:153: ASSERT_EQ(0, strncmp(recv_buf.get(), buffer, buf_len)); On 2013/03/05 20:08:03, Mark Mentovai wrote: > Use memcmp. You have sizes in hand and you want to treat embedded NULs > transparently. Done. https://codereview.chromium.org/12386010/diff/2003/ipc/unix_domain_socket_uti... ipc/unix_domain_socket_util_unittest.cc:156: } On 2013/03/05 20:08:03, Mark Mentovai wrote: > } // namespace Done.
I consider this LGTM-worthy now. Depending on how much changes after this round, you may still want to go for a final review round, but that’s totally at your discretion. Thanks for working on this, it now looks very robust. https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel.h File ipc/ipc_channel.h (right): https://codereview.chromium.org/12386010/diff/2003/ipc/ipc_channel.h#newcode170 ipc/ipc_channel.h:170: // which case the supplied client_euid is updated with it. jeremya wrote: > On 2013/03/05 20:08:03, Mark Mentovai wrote: > > client_euid → peer_euid now. > > I see your unicode arrow and I raise you U+104D ၍ MYANMAR SYMBOL COMPLETED U+1F4A9. https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:55: if (new_fd < 0) Use {} for any multi-line scope, not just multi-statement scopes. https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:81: listen_fd_ = -1; I’d put this right after the close. https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_posix.cc#... ipc/ipc_channel_posix.cc:604: new_pipe < 0) { Are you sure about closing if ServerAcceptConnection returned true but new_pipe < 0 here? https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:40: // We reject socket_name.length() == kMaxSocketNameLength to make room for Thanks. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:89: if (!file_util::CreateDirectory(socket_dir)) { Are you going to add a ScopedAllowIO here? If so, you should add a comment to the header that this does a blocking operation and you have added a ScopedAllowIO. If not, you might still want to add a comment to the header so that callers know that they need to deal with this function potentially blocking on filesystem operations. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:96: PLOG(ERROR) << "unlink " << socket_name; If you’re not going to “return false” for this, is it really worth PLOG(ERROR)? https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:178: return errno == ECONNABORTED || errno == EMFILE || errno == ENFILE || I’m glad to see that you’ve picked out other recoverable errors in addition to my ECONNABORTED example! https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:179: errno == ENOMEM; If you’re checking for ENOMEM, you should probably also tolerate ENOBUFS, which is conceptually very similar. It doesn’t show up in “man 2 accept” on Mac, but it does on Linux and in http://pubs.opengroup.org/onlinepubs/9699919799/functions/accept.html . https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:183: DCHECK(server_socket); This is obsolete now that the next line dereferences server_socket. (This file is a little inconsistent about whether it DCHECKs out-pointers like this right off the bat. I think it’s generally unnecessary. This would favor dumping the DCHECKs on lines 76 and 118 too. But if you really like the DCHECKs, they’re missing on lines 33 and 141.) https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.h (right): https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.h:8: #include <string> C system headers precede C++ system headers, so <sys/types.h> comes before <string>. Put a blank line between the sections for C and C++ headers. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.h:34: // Checks that |peer_fd| belongs to the same user as the running process. “belongs to” is ambiguous. “Checks that the process at the other end of |peer_fd|, which describes a UNIX-domain socket, belongs to the same…” https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util_unittest.cc (right): https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util_unittest.cc:109: if (acceptor_) If you used scoped_ptr and ScopedFD, your destructor would be as simple as unlink. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util_unittest.cc:111: if (client_fd_) And if you don’t use ScopedFD, you need to teach this destructor that 0 is valid for FDs. (If you do use ScopedFD, you still need to teach it that, but only for the server_listen_fd_ check that gates the unlink.) https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util_unittest.cc:121: int client_fd() { return client_fd_; } These two methods can be const (like the one above).
All done! :) Thanks for all your help! https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_factory.cc File ipc/ipc_channel_factory.cc (right): https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:55: if (new_fd < 0) On 2013/03/06 19:34:04, Mark Mentovai wrote: > Use {} for any multi-line scope, not just multi-statement scopes. Done. https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_factory.c... ipc/ipc_channel_factory.cc:81: listen_fd_ = -1; On 2013/03/06 19:34:04, Mark Mentovai wrote: > I’d put this right after the close. Done. https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_posix.cc File ipc/ipc_channel_posix.cc (right): https://codereview.chromium.org/12386010/diff/28001/ipc/ipc_channel_posix.cc#... ipc/ipc_channel_posix.cc:604: new_pipe < 0) { On 2013/03/06 19:34:04, Mark Mentovai wrote: > Are you sure about closing if ServerAcceptConnection returned true but new_pipe > < 0 here? I'm just preserving the behaviour that this had prior to the change, which is that the channel would be closed if accept() < 0. I don't want to make any functional changes to this class. I'm not sure what the reasons might have been behind the decision to Close() on ECONNABORTED. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:89: if (!file_util::CreateDirectory(socket_dir)) { On 2013/03/06 19:34:04, Mark Mentovai wrote: > Are you going to add a ScopedAllowIO here? > > If so, you should add a comment to the header that this does a blocking > operation and you have added a ScopedAllowIO. > > If not, you might still want to add a comment to the header so that callers know > that they need to deal with this function potentially blocking on filesystem > operations. Given that this code path was previously only executed in the service process (used for cloud print) which I imagine doesn't have distinct IO and FILE threads, and the new code path I'm adding for app shims works around the problem by creating the IPC::ChannelFactory on the FILE thread, I think it's safe to leave it without a ScopedAllowIO for now. We may want to add one in the future. For now I'll just add a comment to the header. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:96: PLOG(ERROR) << "unlink " << socket_name; On 2013/03/06 19:34:04, Mark Mentovai wrote: > If you’re not going to “return false” for this, is it really worth PLOG(ERROR)? I think we should probably return false when unlink fails in a non-ENOENT way. Updated. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:179: errno == ENOMEM; On 2013/03/06 19:34:04, Mark Mentovai wrote: > If you’re checking for ENOMEM, you should probably also tolerate ENOBUFS, which > is conceptually very similar. It doesn’t show up in “man 2 accept” on Mac, but > it does on Linux and in > http://pubs.opengroup.org/onlinepubs/9699919799/functions/accept.html . Ah, I was wondering if it existed at all on OSX and whether I would have to #ifdef that bit. Seems to work fine though. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:183: DCHECK(server_socket); On 2013/03/06 19:34:04, Mark Mentovai wrote: > This is obsolete now that the next line dereferences server_socket. > > (This file is a little inconsistent about whether it DCHECKs out-pointers like > this right off the bat. I think it’s generally unnecessary. This would favor > dumping the DCHECKs on lines 76 and 118 too. But if you really like the DCHECKs, > they’re missing on lines 33 and 141.) I like the DCHECKs, it's a clear indication to the reader of the code that the function does not handle the possibility of the out ptr being NULL. I appreciate not having to read the whole function to answer that sort of question :) https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.h (right): https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.h:8: #include <string> On 2013/03/06 19:34:04, Mark Mentovai wrote: > C system headers precede C++ system headers, so <sys/types.h> comes before > <string>. Put a blank line between the sections for C and C++ headers. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... Done. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.h:34: // Checks that |peer_fd| belongs to the same user as the running process. On 2013/03/06 19:34:04, Mark Mentovai wrote: > “belongs to” is ambiguous. “Checks that the process at the other end of > |peer_fd|, which describes a UNIX-domain socket, belongs to the same…” New comment: // Checks that the process on the other end of the UNIX domain socket // represented by |peer_fd| shares the same EUID as this process. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util_unittest.cc (right): https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util_unittest.cc:109: if (acceptor_) On 2013/03/06 19:34:04, Mark Mentovai wrote: > If you used scoped_ptr and ScopedFD, your destructor would be as simple as > unlink. I won't use ScopedFD because I want to unlink() after close(). https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util_unittest.cc:111: if (client_fd_) On 2013/03/06 19:34:04, Mark Mentovai wrote: > And if you don’t use ScopedFD, you need to teach this destructor that 0 is valid > for FDs. > > (If you do use ScopedFD, you still need to teach it that, but only for the > server_listen_fd_ check that gates the unlink.) Done. https://codereview.chromium.org/12386010/diff/28001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util_unittest.cc:121: int client_fd() { return client_fd_; } On 2013/03/06 19:34:04, Mark Mentovai wrote: > These two methods can be const (like the one above). Done.
The tryservers hate it, but I like it. LGTM once you figure that out.
LGTM with a few very minor nits. Hats off for doing this! https://codereview.chromium.org/12386010/diff/60001/ipc/ipc.gypi File ipc/ipc.gypi (right): https://codereview.chromium.org/12386010/diff/60001/ipc/ipc.gypi#newcode76 ipc/ipc.gypi:76: 'ipc_channel_factory.cc', Alphabetical ordering of source files. https://codereview.chromium.org/12386010/diff/60001/ipc/ipc.gypi#newcode83 ipc/ipc.gypi:83: 'ipc_channel_factory.cc', same here https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:56: if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) { HANDLE_EINTR ? https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:191: if (fcntl(accept_fd, F_SETFL, O_NONBLOCK) < 0) { HANDLE_EINTR ? https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.h (right): https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.h:38: // specified by |fd|. If successful, sets |client_euid| to the uid, and returns Just double checking that the variable name is client_euid on purpose?
https://codereview.chromium.org/12386010/diff/60001/ipc/ipc.gypi File ipc/ipc.gypi (right): https://codereview.chromium.org/12386010/diff/60001/ipc/ipc.gypi#newcode76 ipc/ipc.gypi:76: 'ipc_channel_factory.cc', On 2013/03/07 09:42:35, jeremy wrote: > Alphabetical ordering of source files. Done. https://codereview.chromium.org/12386010/diff/60001/ipc/ipc.gypi#newcode83 ipc/ipc.gypi:83: 'ipc_channel_factory.cc', On 2013/03/07 09:42:35, jeremy wrote: > same here Done. https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.cc (right): https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:56: if (fcntl(fd, F_SETFL, O_NONBLOCK) < 0) { On 2013/03/07 09:42:35, jeremy wrote: > HANDLE_EINTR ? Done. https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.cc:191: if (fcntl(accept_fd, F_SETFL, O_NONBLOCK) < 0) { On 2013/03/07 09:42:35, jeremy wrote: > HANDLE_EINTR ? Done. https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... File ipc/unix_domain_socket_util.h (right): https://codereview.chromium.org/12386010/diff/60001/ipc/unix_domain_socket_ut... ipc/unix_domain_socket_util.h:38: // specified by |fd|. If successful, sets |client_euid| to the uid, and returns On 2013/03/07 09:42:35, jeremy wrote: > Just double checking that the variable name is client_euid on purpose? Nope. Fixed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12386010/67001
Retried try job too often on win7_aura for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Message was sent while issue was closed.
Committed patchset #21 manually as r186912 (presubmit successful).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12386010/94001
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12386010/94001
Message was sent while issue was closed.
Change committed as 187233
Message was sent while issue was closed.
This got reverted again -- second time because VMTest failed some automation stuff. I suspect it's the new stuff I put in to test the euid of the server process from the client process. See http://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20%28amd6... for details. Would it be sensible to remove the client-side euid check for now (since if that's a vulnerability, it already existed, but I don't think it is) and make a bug to add it & fix the VMTests? This is slightly complicated by the fact that I have no idea where the code for the VM tests is squirreled away. ChromeOS repos somewhere?
Message was sent while issue was closed.
That sounds reasonable to me but you should also get palmer’s approval.
Message was sent while issue was closed.
I don't understand what would cause the client EUID to != the server EUID. That sounds like a bug in our test automation setup — it's not testing the code as it would run on real computers.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremya@chromium.org/12386010/110002
Message was sent while issue was closed.
Change committed as 187554
Message was sent while issue was closed.
On 2013/03/12 10:12:18, I haz the power (commit-bot) wrote: > Change committed as 187554 Sorry to revert this change. It seems to cause test failures in ipc_tests on Android: http://chromium-build-logs.appspot.com/gtest_query?gtest_query=UnixDomainSock... http://chromium-build-logs.appspot.com/gtest_query?gtest_query=UnixDomainSock... Here is a recent build: http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%... Output: CRITICAL:root:Final result: CRITICAL:root:Crashed: CRITICAL:root:UnixDomainSocketUtil.Connect CRITICAL:root:[FATAL:weak_ptr.cc(16)] Check failed: thread_checker_.CalledOnValidThread() || HasOneRef(). [FATAL:weak_ptr.cc(16)] Check failed: thread_checker_.CalledOnValidThread() || HasOneRef(). Also failures on try bots: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
Message was sent while issue was closed.
Committed patchset #25 manually as r187772 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
