Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9700072:
dbus: add support for passing file descriptors (Closed)
|
Created:
8 years, 9 months ago by Sam Leffler Modified:
8 years, 8 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/git/chromium/src@master Visibility:
Public. |
Descriptiondbus: add support for passing file descriptors
Add support for passing file descriptors in messages.
BUG=chromium-os:27809
TEST=run unit tests
Change-Id: I48e52e52ea1e1a4b96bb0dbec7242337e5871510
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129801
Patch Set 1 #Patch Set 2 : #Patch Set 3 : make support conditional; fix unit test #Patch Set 4 : filter tx/rx of directories; split fd unit tests out and conditionalize for platforms that don't ha… #Patch Set 5 : add dbus::FileDescriptor per keybuk's request #
Total comments: 42
Patch Set 6 : address review comments #Patch Set 7 : more review comments #Patch Set 8 : fix unit test to not run where fd-passing support is missing #
Total comments: 28
Patch Set 9 : more review comments; fill-in unit test #
Total comments: 1
Patch Set 10 : unit test nits #
Messages
Total messages: 37 (0 generated)
Still need to resolve portability issues.
ready
On 2012/03/22 22:39:58, Sam Leffler wrote: > ready I'm not completely clear on how this will work. Will the fd be sent from debugd to the browser process? Will the browser process then prepare information that will be fed to the renderer? The way fd passing in Chrome IPC works is that file fd's are allowed, but directory fd's are explicitly prohibited. If a renderer gains a directory fd, it can break out of the chroot jail by cd'ing to that fd. See: http://code.google.com/searchframe#OAMlx_jo-ck/src/ipc/ipc_channel_posix.cc&e... So, it would be good to add a similar check here: don't let directory fd's go through dbus. It might not be needed in this CL, but it's something to keep in mind.
On 2012/03/22 22:44:55, Jorge Lucangeli Obes wrote: > On 2012/03/22 22:39:58, Sam Leffler wrote: > > ready > > I'm not completely clear on how this will work. Will the fd be sent from debugd > to the browser process? Will the browser process then prepare information that > will be fed to the renderer? The browser creates a pipe and hands the write end to debugd so it can pass back data from it's jailed environment. There is currently no use of receiving an fd and I can remover it if desired (I discussed this with keescook and drewry and the feeling was that if one couldn't inject an fd into the brower w/o cooperation then it was ok to leave the api in). > > The way fd passing in Chrome IPC works is that file fd's are allowed, but > directory fd's are explicitly prohibited. If a renderer gains a directory fd, it > can break out of the chroot jail by cd'ing to that fd. > > See: > http://code.google.com/searchframe#OAMlx_jo-ck/src/ipc/ipc_channel_posix.cc&e... > > So, it would be good to add a similar check here: don't let directory fd's go > through dbus. It might not be needed in this CL, but it's something to keep in > mind. Ok, will add logic to filter fd's in dbus.
On 2012/03/22 22:49:53, Sam Leffler wrote: > On 2012/03/22 22:44:55, Jorge Lucangeli Obes wrote: > > On 2012/03/22 22:39:58, Sam Leffler wrote: > > > ready > > > > I'm not completely clear on how this will work. Will the fd be sent from > debugd > > to the browser process? Will the browser process then prepare information that > > will be fed to the renderer? > > The browser creates a pipe and hands the write end to debugd so it can pass back > data from it's jailed environment. There is currently no use of receiving an fd > and I can remover it if desired (I discussed this with keescook and drewry and > the feeling was that if one couldn't inject an fd into the brower w/o > cooperation then it was ok to leave the api in). Agreed. > > > > The way fd passing in Chrome IPC works is that file fd's are allowed, but > > directory fd's are explicitly prohibited. If a renderer gains a directory fd, > it > > can break out of the chroot jail by cd'ing to that fd. > > > > See: > > > http://code.google.com/searchframe#OAMlx_jo-ck/src/ipc/ipc_channel_posix.cc&e... > > > > So, it would be good to add a similar check here: don't let directory fd's go > > through dbus. It might not be needed in this CL, but it's something to keep in > > mind. > > Ok, will add logic to filter fd's in dbus. Can the renderers access the dbus subsystem at all? The worry here is that a renderer might get a dir fd. If that cannot happen, then there might not be a need for this. However, since the check shouldn't be complicated, it might very well be better to be consistent with the Chrome IPC. Thanks!
PTAL
lgtm
I'm concerned about the approach of PopFileDescriptor() returning an open file
descriptor as an integer, rather than a wrapped resource.
Consider the following code, which is easy to think you can do:
if (!reader->PopString(&name) ||
!reader->PopFileDescriptor(&fd) ||
!reader->PopUint32(&flags)) {
Now you have a problem if the flags are missing from the message, you leaked a
file descriptor!
This kind of thing is what RAII is designed for, I'd be much more comfortable
with PopFileDescriptor setting a type of dbus::FileDescriptor, which when
deleted automatically closes the file descriptor associated.
Obtaining the file descriptor from that object without it being closed on delete
is left as an exercise for you ;-) Likewise creating an object of that type to
pass to AppendFileDescriptor that doesn't close the one you passed
PTAL
http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.cc File dbus/file_descriptor.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.cc#new... dbus/file_descriptor.cc:15: } // namespace dbus nit: two spaces before // http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h File dbus/file_descriptor.h (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:19: FileDescriptor() : value_(-1), owner_(true) {} owner_(true) here means you'll close(-1) later; better to just set owner_(false), since it's explicitly set to true by PutValue() anyway http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode977 dbus/message.cc:977: } doesn't close the file descriptor before returning also what if base::PlatformFileInfo constructor, base::GetPlatformFileInfo or LOG() throws an exception? http://codereview.chromium.org/9700072/diff/15001/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:114: EXPECT_EQ(1, fd_value.value()); does this pass? I would be hella-surprised if the returned file descriptor was actually == 1, it should be a new file descriptor referring to the same underlying file http://codereview.chromium.org/9700072/diff/15001/dbus/values_util.cc File dbus/values_util.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/values_util.cc#newcode168 dbus/values_util.cc:168: // Cannot distinguish fd from an int at the moment Sure you can, it's a dbus::FileDescriptor now ;-)
http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.cc File dbus/file_descriptor.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.cc#new... dbus/file_descriptor.cc:15: } // namespace dbus copied from object_path; fixed http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode977 dbus/message.cc:977: } Not sure where you're referring. fd is not closed because it's returned via value->PutValue. The error case will abort the process because LOG(FATAL) does that. http://codereview.chromium.org/9700072/diff/15001/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:114: EXPECT_EQ(1, fd_value.value()); It did pass but what you say seems right so let me re-check. http://codereview.chromium.org/9700072/diff/15001/dbus/values_util.cc File dbus/values_util.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/values_util.cc#newcode168 dbus/values_util.cc:168: // Cannot distinguish fd from an int at the moment Previously I took the fd and stashed it in an integer Value but then the caller can't distinguish it from another integer Value. I'm not sure it's worth adding it right now but rather than explicitly leak the fd I changed this code.
http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.cc File dbus/file_descriptor.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.cc#new... dbus/file_descriptor.cc:11: if (owner_) If we get rid of owner_, this becomes if (value_ == -1) http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h File dbus/file_descriptor.h (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:17: // Permit initialization without a value for passing to nit: Permit -> Permits. Please also fix other places. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:20: explicit FileDescriptor(int value) : value_(value), owner_(false) {} Could you remove the default constructor? The style guide discourages function overloading, including constructors http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl... http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:28: void PutValue(int value) { Put -> Set? Set is more common for this. http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:34: int TakeValue(void) { nit: (void) -> () http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:36: return value_; I guess owner_ is unnecessary. We can use -1 as non-owner. int copy = value_; value_ = 1; retrn copy; http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:37: } nit: we usually have a blank line here. http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode205 dbus/message.cc:205: #if defined(DBUS_TYPE_UNIX_FD) Hmm, there seems to be many #ifdefs based on DBUS_TYPE_UNIX_FD added in this patch. I'd rather like to minimize #ifdefs. What about doing this in message.h: // DBUS_TYPE_UNIX_FD was added in D-Bus version x.x.x #if defined(DBUS_TYPE_UNIX_FD) const bool kDBusTypeUnixFdIsSupported = true; #else const bool kDBusTypeUnixFdIsSupported = false; #define DBUS_TYPE_UNIX_FD ((int) 'h') #endif Then, we can use kDBusTypeUnixFdIsSupported like CHECK(kDBusTypeUnixFdIsSupported); http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode693 dbus/message.cc:693: void MessageWriter::AppendFileDescriptor(const FileDescriptor& value) { If we are to go with the idea described above, you can just put CHECK(kDBusTypeUnixFdIsSupported); and get rid of the #ifdef. http://codereview.chromium.org/9700072/diff/15001/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.h#newcode71 dbus/message.h:71: #if defined(DBUS_TYPE_UNIX_FD) Please add some comment that DBUS_TYPE_UNIX_FD is not defined in older versions of D-bus. Would be nice if we can say it's added in the version xxx. http://codereview.chromium.org/9700072/diff/15001/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:99: TEST(MessageTest, AppendAndPopFileDescriptor) { If we are to go with kDBusTypeUnixFdIsSupported, you can add if (!kDBusTypeUnixFdIsSupported) return; and get rid of #ifdef around the function. http://codereview.chromium.org/9700072/diff/15001/dbus/values_util.cc File dbus/values_util.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/values_util.cc#newcode168 dbus/values_util.cc:168: // Cannot distinguish fd from an int at the moment On 2012/03/27 23:20:40, Sam Leffler wrote: > Previously I took the fd and stashed it in an integer Value but then the caller > can't distinguish it from another integer Value. I'm not sure it's worth adding > it right now but rather than explicitly leak the fd I changed this code. NOTREACHED() sounds reasonable to me.
Neutered the unit test for now because (as keybuk pointed out) it was wrong. The right fix is messy and the test is not run on any of the bots because they lack a sufficiently new dbus to enable fd-passing support. Will open an issue to track fixing the unit test (not sure whether it's ok to drop fstat's et al in there). http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.cc File dbus/file_descriptor.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.cc#new... dbus/file_descriptor.cc:11: if (owner_) Please explain how to get rid of owner_. instances may have a valid value_ and have owner_ true or false. http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h File dbus/file_descriptor.h (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:17: // Permit initialization without a value for passing to Not sure which clause you're pointed me at but "Permit" is correct AFAIK. http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:20: explicit FileDescriptor(int value) : value_(value), owner_(false) {} This cribs from dbus::ObjectPath. Removing it makes usage more awkward because every instance must be done with an explicit -1 argument. I understand the intent of the style guideline but do we really want it here? http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:28: void PutValue(int value) { Please get together w/ keybuk and decide which way you want me to shave this yak? http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:34: int TakeValue(void) { On 2012/03/28 00:32:43, satorux1 wrote: > nit: (void) -> () Done. http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:36: return value_; They are not equivalent. It is useful to preserve the value while releasing ownership. http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:37: } On 2012/03/28 00:32:43, satorux1 wrote: > nit: we usually have a blank line here. Done. http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode205 dbus/message.cc:205: #if defined(DBUS_TYPE_UNIX_FD) You cannot do this unless you define UNIX_FD in the unsupported case. There are 4 #ifdefs in this file. 1 is required because UNIX_FD is undefined otherwise. 2 can be handled as you request but result in a bunch of code being compiled in that will never be used/work. 1 is around the #include of platform_file.h that I intentionally bracketed to show it was only needed for UNIX_FD support (maybe a style violation?). http://codereview.chromium.org/9700072/diff/15001/dbus/message.h File dbus/message.h (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.h#newcode71 dbus/message.h:71: #if defined(DBUS_TYPE_UNIX_FD) On 2012/03/28 00:32:43, satorux1 wrote: > Please add some comment that DBUS_TYPE_UNIX_FD is not defined in older versions > of D-bus. Would be nice if we can say it's added in the version xxx. Done. http://codereview.chromium.org/9700072/diff/15001/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:99: TEST(MessageTest, AppendAndPopFileDescriptor) { Unless kDBusTypeUnixFdIsSupported is defined in message.h or similar I'll be replacing 1 #if by 6+ lines of boilerplate.
http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h File dbus/file_descriptor.h (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:17: // Permit initialization without a value for passing to On 2012/03/28 17:28:09, Sam Leffler wrote: > Not sure which clause you're pointed me at but "Permit" is correct AFAIK. These comments should be descriptive ("Opens the file") rather than imperative ("Open the file"); the comment describes the function
http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode205 dbus/message.cc:205: #if defined(DBUS_TYPE_UNIX_FD) On 2012/03/28 17:28:09, Sam Leffler wrote: > You cannot do this unless you define UNIX_FD in the unsupported case. My proposal was to define DBUS_TYPE_UNIX_FD for the unsupported case. Please see my original comment. There are > 4 #ifdefs in this file. 1 is required because UNIX_FD is undefined otherwise. 2 > can be handled as you request but result in a bunch of code being compiled in > that will never be used/work. I'm fine with that. The unused code is fairly small, but #ifdefs make code harder to maintain. IMHO, the latter is worse in this case. > 1 is around the #include of platform_file.h that > I intentionally bracketed to show it was only needed for UNIX_FD support (maybe > a style violation?).
http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode977 dbus/message.cc:977: } it does that now, it might not in future; likewise exception handling... basically you can leak an fd between :968 and :978 if things change under you, your code should defend against that
http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h File dbus/file_descriptor.h (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:24: // Retrieve value as an int. nit: Retrieves. Also explain that this does not take ownership. http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:27: // Set the value and assign ownership. nit: Sets http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:28: void PutValue(int value) { I asked Sam to change this from Set to Put because the object is taking ownership of the fd you pass in, rather than just setting the value contained within it. ie. Put/Take rather than Set/Get http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:36: return value_; It's probably worth explaining in the class comment what ownership implies, and give the example of the non-ownership case (passing a dbus::FileDescriptor you don't want closed) and the ownership case (receiving a dbus::FileDescriptor you do)
PTAL http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h File dbus/file_descriptor.h (right): http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:17: // Permit initialization without a value for passing to On 2012/03/28 17:30:16, satorux1 wrote: > On 2012/03/28 17:28:09, Sam Leffler wrote: > > Not sure which clause you're pointed me at but "Permit" is correct AFAIK. > > These comments should be descriptive ("Opens the file") rather than imperative > ("Open the file"); the comment describes the function Done. http://codereview.chromium.org/9700072/diff/15001/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:36: return value_; On 2012/03/28 17:57:37, keybuk wrote: > It's probably worth explaining in the class comment what ownership implies, and > give the example of the non-ownership case (passing a dbus::FileDescriptor you > don't want closed) and the ownership case (receiving a dbus::FileDescriptor you > do) Done. http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode205 dbus/message.cc:205: #if defined(DBUS_TYPE_UNIX_FD) On 2012/03/28 17:41:54, satorux1 wrote: > On 2012/03/28 17:28:09, Sam Leffler wrote: > > You cannot do this unless you define UNIX_FD in the unsupported case. > > My proposal was to define DBUS_TYPE_UNIX_FD for the unsupported case. Please see > my original comment. > > > > There are > > 4 #ifdefs in this file. 1 is required because UNIX_FD is undefined otherwise. > 2 > > can be handled as you request but result in a bunch of code being compiled in > > that will never be used/work. > > I'm fine with that. The unused code is fairly small, but #ifdefs make code > harder to maintain. IMHO, the latter is worse in this case. > > > > > 1 is around the #include of platform_file.h that > > I intentionally bracketed to show it was only needed for UNIX_FD support > (maybe > > a style violation?). > Done. http://codereview.chromium.org/9700072/diff/15001/dbus/message.cc#newcode977 dbus/message.cc:977: } On 2012/03/28 17:53:12, keybuk wrote: > it does that now, it might not in future; likewise exception handling... > basically you can leak an fd between :968 and :978 if things change under you, > your code should defend against that Done.
http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:117: // TODO(sleffler) need to fstat each fd to compare This should be trivial enough, fstat will get you a matching rdev right? Worth doing before committing so the test tests the right thing
http://codereview.chromium.org/9700072/diff/20002/dbus/file_descriptor.h File dbus/file_descriptor.h (right): http://codereview.chromium.org/9700072/diff/20002/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:25: // will not automatically close "1" because it is not owned. Oh I see. This comment explains why owner_ is needed. Thanks. http://codereview.chromium.org/9700072/diff/20002/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:39: void PutValue(int value) { I found it confusing. For this, TakeValue() sounds more natural to me, as *this object* takes the ownership. I'd go with PutValue(int) -> Take(int) TakeValue() -> Release(). http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode206 dbus/message.cc:206: FileDescriptor value; nit: value -> file_descriptor? http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode210 dbus/message.cc:210: base::StringPrintf("%u", value.value()) + "\n"; nit: %d? fd is an int. http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode697 dbus/message.cc:697: if (!ok || info.is_directory) These are different errors. !ok means it just failed to get the file info, which likely means the fd is invalid. LOG(FATAL) in this case is rather questionable. Trying to send an invalid fd is a bad idea, but it's the caller's fault, so I'd go with something like: if (ok && info.is_directory) LOG(FATAL) << "Attempt to pass invalid file descriptor"; // |ok| can be false, which means |fd| is invalid, but it's caller's fault, // so we don't stop. http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode698 dbus/message.cc:698: LOG(FATAL) << "Attempt to pass invalid file descriptor"; Please add some comment why we are disallowing the directory fd passing. otherwise, it looks rather cryptic http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode964 dbus/message.cc:964: int fd; nit: initialize it to -1 to be extra defensive. http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode971 dbus/message.cc:971: if (!ok || info.is_directory) { ditto. these are different errors. http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:43: // Append 0, 1, 2, 3, 4, 5, 6, 7, 8, "string", "/object/path", stdout. nit: remove ", stdout" http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:100: return; Could you add some logging like if (!dbus::kDBusTypeUnixFdIsSupported) { LOG(WARNING) << "FD passing is not supported"; return; } so we can tell if the test is run or not. http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:117: // TODO(sleffler) need to fstat each fd to compare Why cannot we do EXPECT_EQ(1, fd_value.value())? Here, we are just storing 1 in DBusMessage and popping from it. I thought dbus_message_iter_append_basic() and dbus_message_iter_get_basic() just preserved 1 as-is, but I may be wrong. I didn't check the implementation. Would be nice to leave some comment why we are not checking fd_value.value() here. http://codereview.chromium.org/9700072/diff/20002/dbus/values_util.cc File dbus/values_util.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/values_util.cc#newcode167 dbus/values_util.cc:167: // Cannot distinguish fd from an int at the moment maybe drop "at the moment". base::Value won't have a file descriptor type in the future.
http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:117: // TODO(sleffler) need to fstat each fd to compare No, dbus calls dup() all over the place like crazy on those fds so it can deal with ownership in its own way It may be that it comes out as 1, but that might be just by luck on some phases of the moon (and it didn't when I tested here with a debug version of dbus).
http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:117: // TODO(sleffler) need to fstat each fd to compare On 2012/03/28 20:56:18, keybuk wrote: > No, dbus calls dup() all over the place like crazy on those fds so it can deal > with ownership in its own way > > It may be that it comes out as 1, but that might be just by luck on some phases > of the moon (and it didn't when I tested here with a debug version of dbus). Then, we need some comment about it. :)
http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode697 dbus/message.cc:697: if (!ok || info.is_directory) On 2012/03/28 20:49:37, satorux1 wrote: > These are different errors. !ok means it just failed to get the file info, which > likely means the fd is invalid. LOG(FATAL) in this case is rather questionable. > > Trying to send an invalid fd is a bad idea, but it's the caller's fault, so I'd > go with something like: > > if (ok && info.is_directory) > LOG(FATAL) << "Attempt to pass invalid file descriptor"; > // |ok| can be false, which means |fd| is invalid, but it's caller's fault, > // so we don't stop. And in this case, we can display an error message specifically pointing out the fact that this is a dir fd, and we don't want to send those around.
one more time... http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc File dbus/message.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode206 dbus/message.cc:206: FileDescriptor value; On 2012/03/28 20:49:37, satorux1 wrote: > nit: value -> file_descriptor? Done. http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode210 dbus/message.cc:210: base::StringPrintf("%u", value.value()) + "\n"; On 2012/03/28 20:49:37, satorux1 wrote: > nit: %d? fd is an int. Done. http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode697 dbus/message.cc:697: if (!ok || info.is_directory) Yes they are different errors; the message is intentionally general to cover both cases. I think it is a bad idea to carry on with a known bad file descriptor even though it is the fault of the caller. I expect the dbus library will abort if handed an invalid descriptor but there's no guarantee and if it continues on then I can imaging difficult-to-debug problems that can be trivially caught here. I can spend some more code to print out the fd or treat the two cases separately but not catching it here seems bad. http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode698 dbus/message.cc:698: LOG(FATAL) << "Attempt to pass invalid file descriptor"; On 2012/03/28 20:49:37, satorux1 wrote: > Please add some comment why we are disallowing the directory fd passing. > otherwise, it looks rather cryptic Done. http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode964 dbus/message.cc:964: int fd; On 2012/03/28 20:49:37, satorux1 wrote: > nit: initialize it to -1 to be extra defensive. Done. http://codereview.chromium.org/9700072/diff/20002/dbus/message.cc#newcode971 dbus/message.cc:971: if (!ok || info.is_directory) { see above http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:43: // Append 0, 1, 2, 3, 4, 5, 6, 7, 8, "string", "/object/path", stdout. On 2012/03/28 20:49:37, satorux1 wrote: > nit: remove ", stdout" Done. http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:100: return; On 2012/03/28 20:49:37, satorux1 wrote: > Could you add some logging like > > if (!dbus::kDBusTypeUnixFdIsSupported) { > LOG(WARNING) << "FD passing is not supported"; > return; > } > > so we can tell if the test is run or not. Done. http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:117: // TODO(sleffler) need to fstat each fd to compare Implemented the test case. I used fstat calls which I assume are ok since this code is only built on posix systems. If not then we'll need to figure out a way to #ifdef the code (since we can't use DBUS_TYPE_UNIX_FD). http://codereview.chromium.org/9700072/diff/20002/dbus/values_util.cc File dbus/values_util.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/values_util.cc#newcode167 dbus/values_util.cc:167: // Cannot distinguish fd from an int at the moment On 2012/03/28 20:49:37, satorux1 wrote: > maybe drop "at the moment". base::Value won't have a file descriptor type in the > future. Done.
LGTM, but please wait for LGTM from keybuk. http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/20002/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:117: // TODO(sleffler) need to fstat each fd to compare On 2012/03/28 22:06:33, Sam Leffler wrote: > Implemented the test case. I used fstat calls which I assume are ok since this > code is only built on posix systems. If not then we'll need to figure out a way > to #ifdef the code (since we can't use DBUS_TYPE_UNIX_FD). No need to worry about #ifdef. This only compiles for Linux and Chrome OS. http://codereview.chromium.org/9700072/diff/22001/dbus/message_unittest.cc File dbus/message_unittest.cc (right): http://codereview.chromium.org/9700072/diff/22001/dbus/message_unittest.cc#ne... dbus/message_unittest.cc:122: int status_stdout = ::fstat(1, &sb_stdout); nit: is :: needed? wrapping it with HANDLE_EINTR in base/eintr_wrapper.h would be good.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sleffler@chromium.org/9700072/6015
Try job failure for 9700072-6015 (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sleffler@chromium.org/9700072/6015
http://codereview.chromium.org/9700072/diff/20002/dbus/file_descriptor.h File dbus/file_descriptor.h (right): http://codereview.chromium.org/9700072/diff/20002/dbus/file_descriptor.h#newc... dbus/file_descriptor.h:39: void PutValue(int value) { On 2012/03/28 20:49:37, satorux1 wrote: > I found it confusing. For this, TakeValue() sounds more natural to me, as *this > object* takes the ownership. I'd go with > > PutValue(int) -> Take(int) > TakeValue() -> Release(). This question wasn't answered. I wanted it to be fixed...
Sorry, saw your lgtm and thought you'd changed your mind. I find Take/Release to be confusing (it inverts the "role"). Since you don't agree w/ what keybuk and I came up with perhaps we can just go back to Set/Get? (i.e. Put -> Set, Take -> Get)? Otherwise perhaps you have another suggestion?
I thought we generally name method names as actions performed by the object in OOP. Here, FileDescriptor takes ownership, and releases it, so I thought Take() and Release() would look more natural: FileDescriptor file_descriptor; file_descriptor.Take(fd); // file_descriptor takes the ownership. fd = file_descriptor.Release(); // file_descriptor releases it. I also had scoped_ptr's release() in mind, which releases the ownership.
On 2012/03/29 17:42:11, satorux1 wrote: > I thought we generally name method names as actions performed by the object in > OOP. Here, FileDescriptor takes ownership, and releases it, so I thought Take() > and Release() would look more natural: > If method names are actions performed by the object, Get() would mean that the object got something, not that you got something.
On 2012/03/29 17:56:51, keybuk wrote: > On 2012/03/29 17:42:11, satorux1 wrote: > > I thought we generally name method names as actions performed by the object in > > OOP. Here, FileDescriptor takes ownership, and releases it, so I thought > Take() > > and Release() would look more natural: > > > > If method names are actions performed by the object, Get() would mean that the > object got something, not that you got something. That's a strong argument. I thought following scoped_ptr's release() convention was nice, but I don't insist. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sleffler@chromium.org/9700072/6015
Change committed as 129801 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
