Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18337010:
Avoid std::string copying in GetFileNameInWhitelist. (Closed)
|
Created:
7 years, 5 months ago by Jorge Lucangeli Obes Modified:
7 years, 5 months ago Reviewers:
jln (very slow on Chromium) CC:
chromium-reviews, agl, jln+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAvoid std::string copying in GetFileNameInWhitelist.
BUG=256452
TEST=sandbox_linux_unittests passes.
TEST=about:gpu shows "Sandboxed: true", browser works.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210651
Patch Set 1 #
Total comments: 21
Patch Set 2 : Address most reviewer comments, missing unit tests and comments. #Patch Set 3 : Use libc's strcmp(3). #
Total comments: 2
Patch Set 4 : Make the loop saner. #Patch Set 5 : Small whitespace consistency fix. #Messages
Total messages: 9 (0 generated)
Not sure whether using vector::iterator directly is enough, but this should get rid of all implicit string allocs.
Let's just do the strcmp (with unit tests) here ? Find should be done inline in the broker process, and I plan to replace the vectors to have real async signal safety anyways. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.cc (left): https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:451: int requested_mode, const char** file_to_access) const { Why the re-indent? https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:497: const char** file_to_open) const { Why the re-indent? https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process_utils.cc (right): https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:11: const char* BrokerProcessUtils::Find( If this is not really guaranteed async signal safe. More importantly, it means that in the broker process, the paranoid part where we always return a string from the whitelist, and never the one provided (which is a security in depth measure in case strcmp is broken) is lost). I would not make it part of this class. Just put it back inline where it was? https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:24: int BrokerProcessUtils::StrCmp(const char* s1, const char* s2) { As mentioned, this really needs unit tests. Please, have at least one where one of the strings is infinite: fill a page with 'A' and have the next page PROT_NONE to trigger a segfault. (Or if you don't want to spend time doing this, it's also ok to just rely on ASAN checking this for you. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:35: if (*s1 == '\0' && *s2 == '\0') Style: use brackets everywhere if you use them in the last else {} https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:42: NOTREACHED(); This is not async signal safe. Use RAW_CHECK (in !NDEBUG). https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:43: return 0; GCC is still dumb enough to not see this dead code and require you to put a return here (pretty sure Clang is ok)? If yes, put a comment maybe ? https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process_utils.h (right): https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.h:12: Add some documentation about the class and about the fact that it's designed to be async signal safe. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.h:13: class BrokerProcessUtils { You absolutely need unit tests as well! https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.h:13: class BrokerProcessUtils { Let's name it something less specific, like SandboxUtils maybe ? https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.h:16: const std::vector<std::string>& haystack); Don't forget to document these functions, even if it seems pretty straightforward. Especially, mention async signal safety. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.h:18: }; Don't forget DISALLOW_IMPLICIT_CONSTRUCTORS
Thinking about ways to simplify this CL: it probably doesn't make sense for now to a StrCmp rewritten since it's a lot more unlikely to be signal unsafe that the rest that we're doing. So feel free to just call libc's strcmp here and we can revise this decision once I land the no vector patch. Your choice.
This has replies from comments in PS1 but I'll reupload with libc's strcmp to unblock James. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.cc (left): https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:451: int requested_mode, const char** file_to_access) const { On 2013/07/08 22:36:51, Julien Tinnes wrote: > Why the re-indent? Done. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process.cc:497: const char** file_to_open) const { On 2013/07/08 22:36:51, Julien Tinnes wrote: > Why the re-indent? Done. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process_utils.cc (right): https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:11: const char* BrokerProcessUtils::Find( On 2013/07/08 22:36:51, Julien Tinnes wrote: > If this is not really guaranteed async signal safe. More importantly, it means > that in the broker process, the paranoid part where we always return a string > from the whitelist, and never the one provided (which is a security in depth > measure in case strcmp is broken) is lost). > > I would not make it part of this class. > > Just put it back inline where it was? Done, but I don't understand why this would not return a string from the whitelist, since the only two return statements are "return it->c_str()" and "return NULL". https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:35: if (*s1 == '\0' && *s2 == '\0') On 2013/07/08 22:36:51, Julien Tinnes wrote: > Style: use brackets everywhere if you use them in the last else {} Done. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:42: NOTREACHED(); On 2013/07/08 22:36:51, Julien Tinnes wrote: > This is not async signal safe. Use RAW_CHECK (in !NDEBUG). Done. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.cc:43: return 0; On 2013/07/08 22:36:51, Julien Tinnes wrote: > GCC is still dumb enough to not see this dead code and require you to put a > return here (pretty sure Clang is ok)? If yes, put a comment maybe ? Done. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... File sandbox/linux/services/broker_process_utils.h (right): https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.h:13: class BrokerProcessUtils { On 2013/07/08 22:36:51, Julien Tinnes wrote: > Let's name it something less specific, like SandboxUtils maybe ? Done. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.h:16: const std::vector<std::string>& haystack); On 2013/07/08 22:36:51, Julien Tinnes wrote: > Don't forget to document these functions, even if it seems pretty > straightforward. Especially, mention async signal safety. Done. https://chromiumcodereview.appspot.com/18337010/diff/1/sandbox/linux/services... sandbox/linux/services/broker_process_utils.h:18: }; On 2013/07/08 22:36:51, Julien Tinnes wrote: > Don't forget DISALLOW_IMPLICIT_CONSTRUCTORS Done.
Updated and greatly simplified, PTAL.
lgtm with mandatory nit. (If I'm around, feel free to let me take another quick look). https://chromiumcodereview.appspot.com/18337010/diff/8002/sandbox/linux/servi... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/18337010/diff/8002/sandbox/linux/servi... sandbox/linux/services/broker_process.cc:72: if (strcmp(requested_filename, it->c_str()) == 0) This is strange, you're iterating over all file names and you don't stop when you find something? Eliminate found_filename. Just put the section below (if (file_to_open) {} return true) here.
https://chromiumcodereview.appspot.com/18337010/diff/8002/sandbox/linux/servi... File sandbox/linux/services/broker_process.cc (right): https://chromiumcodereview.appspot.com/18337010/diff/8002/sandbox/linux/servi... sandbox/linux/services/broker_process.cc:72: if (strcmp(requested_filename, it->c_str()) == 0) On 2013/07/09 05:36:40, Julien Tinnes wrote: > This is strange, you're iterating over all file names and you don't stop when > you find something? > > Eliminate found_filename. Just put the section below (if (file_to_open) {} > return true) here. This is what happens when one codes at 9 pm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jorgelo@chromium.org/18337010/23001
Message was sent while issue was closed.
Change committed as 210651 |
