Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18656004:
Added a new SafeSPrintf() function that implements snprintf() in an async-safe-fashion (Closed)
|
Created:
7 years, 5 months ago by Markus (顧孟勤) Modified:
7 years, 3 months ago Reviewers:
Jeffrey Yasskin, jln (very slow on Chromium), willchan no longer on Chromium, rvargas (doing something else) CC:
chromium-reviews, agl, jln+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded a new base::strings::SafeSPrintf() function that can
safely be called from all restricted (e.g. async-signal-safe)
environments. It is roughly equivalent to snprintf(). But
it is easier to use, as it uses C++ to be type-safe.
In release builds, this function is guaranteed to never call
any other library function nor any system calls. In debug
builds, we call RAW_CHECK() in some circumstances.
This code is needed for (at least) the seccomp sandbox and
to clean up code in the stack tracer. Those changes are
pending and will be submitted as separate CLs.
BUG=none
TEST=base_unittests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220746
Patch Set 1 #Patch Set 2 : Variadic templates make the code even cleaner (for C++-11) #Patch Set 3 : Clean up DieFormatHelper() and fold SANDBOX_XXX_FORMATTED() into SANDBOX_XXX() #Patch Set 4 : After the last refactoring, we can actually move the helper back into the outer "Die" class. #Patch Set 5 : Moved into base/debug/format.*; added missing features needed by stack tracer; added unittest #Patch Set 6 : Fixed reference value in unittest #
Total comments: 13
Patch Set 7 : Addressed Jeffrey's comments #
Total comments: 15
Patch Set 8 : Refactored to include just the new Format() function, but none of its users #Patch Set 9 : Addressed Julien's comments #
Total comments: 9
Patch Set 10 : Refactored buffer management to address the first part of jln's comments and allow him to add more … #Patch Set 11 : Added missing test, more clean ups requested by jln, more comments, support for octals #
Total comments: 53
Patch Set 12 : Addressed jln's comments #
Total comments: 3
Patch Set 13 : Disabled death tests on Android and added myself to OWNERS #Patch Set 14 : Addressed jln's remaining comments #Patch Set 15 : Moved to base/strings/ and renamed to SafeSPrintf() #
Total comments: 20
Patch Set 16 : Addressed Will's comments; renamed files to base/strings/safe_sprintf.* #
Total comments: 10
Patch Set 17 : Renamed some identifiers (prompted by rvargas' comments) #Patch Set 18 : Refined comments at the top of safe_sprintf.h #
Total comments: 2
Patch Set 19 : Minor nits #
Messages
Total messages: 34 (0 generated)
Julien, this CL actually turned out to be more complicated than I originally anticipated. It turns out, we didn't have support for printing formatted messages from SANDBOX_DIE(). And we really don't want to call into libc, as we are in a potentially broken state and need to be super careful about a) our code being async- signal-safe, and b) avoiding system calls that could accidentally raise recursive SIGSYS signals. I now added a new SANDBOX_DIE_FORMATTED() macro. It is async- signal safe (in fact, it doesn't do any system or library calls to format the error message). Additionally, this macro is type-safe, which comes in really handy when trying to print arguments that can have unexpected byte widths and/or signed-ness. Much nicer to use than snprintf(). It looks complicated, but the C++ optimizer eliminates almost all of the extra code at compile-time. It's actually overall pretty efficient. I haven't had a chance to write unittests yet; so, this CL is obviously incomplete. I just wanted to give you a heads-up, so you can let me know if this is generally what we want to do here. I'll also add more unittests for Trap(), if I can think of some new tests that would stress this part of the system.
On 2013/07/13 09:32:35, Markus (顧孟勤) wrote: > Julien, this CL actually turned out to be more complicated than > I originally anticipated. It turns out, we didn't have support > for printing formatted messages from SANDBOX_DIE(). And we really > don't want to call into libc, as we are in a potentially broken > state and need to be super careful about a) our code being async- > signal-safe, and b) avoiding system calls that could accidentally > raise recursive SIGSYS signals. > > I now added a new SANDBOX_DIE_FORMATTED() macro. It is async- > signal safe (in fact, it doesn't do any system or library calls to > format the error message). Additionally, this macro is type-safe, > which comes in really handy when trying to print arguments that > can have unexpected byte widths and/or signed-ness. Much nicer to > use than snprintf(). > > It looks complicated, but the C++ optimizer eliminates almost all > of the extra code at compile-time. It's actually overall pretty > efficient. > > I haven't had a chance to write unittests yet; so, this CL is > obviously incomplete. I just wanted to give you a heads-up, so you > can let me know if this is generally what we want to do here. > > I'll also add more unittests for Trap(), if I can think of some > new tests that would stress this part of the system. High level review (sorry for being so insanely late): async-signal safe snprintf is very useful to have. Do you mind putting it in its own file (with its own unit tests) ? I think it'll also make unit testing a lot easier. Ideally I would love to have this in base/, there is already some async-signal safe code there that could benefit from being centralized. Look at base/debug/stack_trace.h. I think itoa_r is already your code that someone put there. Alternatively, you could put it in sandbox/linux/services/
I ended up moving the snprintf() implementation into base::debug::Format(). I then noticed that stacktrace_posix.cc had (almost) copied-and-pasted our version of itoa_r(), but introduced a few subtle bugs and/or usage restrictions. So, I converted it to using Format() instead. That also allowed me to do a few other small cleanups in the stack dumper. I verified that the unittest still works correctly after making these changes. In order to do make Format() usable for the stack dumper, I had to make it a lot more generic though. Most importantly, I had to teach it about padding of arguments (e.g. %3d, instead of just %d). That's probably a good thing though, if we want this code to be used by others. It still doesn't have *all* the features of snprintf(), but I feel it has all the important ones. And because of type-safety, it is actually much easier to use. I also was extra paranoid about things like integer overflows. The code is still relatively compact, but it now has enough different states that it is certainly possible to introduce subtle bugs. I think I got pretty close to 100% test coverage. But I am sure you'll correct me, if you spot something that I missed. Once you are happy with this CL, we can add extra reviewers/owners for the code in base/... At that point, I'll probably also split out the changes to the stack dumper into a separate CL.
I added Jeffrey as a reviewer for C++ issues. Feel free to review more if you feel like it. But if you just do C++, that's really good enough. Julien will do the overall review for the entire CL. And once you two are generally happy, I'll ask OWNERS to chime in on whether they are OK with this CL, and/or whether things need to be moved around a little bit. Of course, if you do have an opinion about that, do chime in.
The C++ side lgtm. https://codereview.chromium.org/18656004/diff/69001/base/debug/format.cc File base/debug/format.cc (right): https://codereview.chromium.org/18656004/diff/69001/base/debug/format.cc#newc... base/debug/format.cc:38: if (*count > std::numeric_limits<ssize_t>::max() - inc) { I think this will give the wrong answer if inc>std::numeric_limits<ssize_t>::max(). https://codereview.chromium.org/18656004/diff/69001/base/debug/format.cc#newc... base/debug/format.cc:124: if (i == -i) { That's undefined behavior for signed numbers (sorry); compute the negation as unsigned instead or use numeric_limits<int64_t>::min(). https://codereview.chromium.org/18656004/diff/69001/base/debug/format.cc#newc... base/debug/format.cc:141: // don't allow more than MAXINT bytes. MAXINT != numeric_limits<ssize_t>::max(). SSIZE_MAX instead? (Here and in the header comment.) https://codereview.chromium.org/18656004/diff/69001/base/debug/format.cc#newc... base/debug/format.cc:152: // memmove(start, start+1, --ptr - start) This is weird. Why do you need to erase the first digit you printed? https://codereview.chromium.org/18656004/diff/69001/base/debug/format.cc#newc... base/debug/format.cc:161: // Output the next digit and (if necessary) compensate for the lowest- "lowest-most negative integer" should probably be "lowest integer" or "most-negative integer". https://codereview.chromium.org/18656004/diff/69001/base/debug/format.cc#newc... base/debug/format.cc:329: i &= ~(static_cast<int64_t>(-1) << (8*arg.width_)); Heh, alternately, "i &= (1LL << 8*arg.width_) - 1". You could handle this in the Arg() constructor by converting unsigned arguments to uint64_t before storing them in the int64_t. https://codereview.chromium.org/18656004/diff/69001/base/debug/format.cc#newc... base/debug/format.cc:419: // signal-safe, we are limited in the type of error handling that we Is abort() or "*(volatile char*)0 = 0" ok for debug mode? https://codereview.chromium.org/18656004/diff/69001/base/debug/format.h File base/debug/format.h (right): https://codereview.chromium.org/18656004/diff/69001/base/debug/format.h#newco... base/debug/format.h:40: // %c, %d, %x, %X, %p, and %s. Do you require %c to match characters or can it take any integer? Do you require %d to match signed integers or can it convert unsigned to signed? Do you require %{x,X} to match unsigned integers, or can they convert signed integers? https://codereview.chromium.org/18656004/diff/69001/base/debug/format.h#newco... base/debug/format.h:72: // sz = FormatN(buf, sz, "Error message \"%s\"\n", err); The argument should be sz+1, right? (This interface seems hard to use. Yes, it matches snprintf, but why not return the length you actually need to allocate?) https://codereview.chromium.org/18656004/diff/69001/base/debug/format.h#newco... base/debug/format.h:131: ssize_t FormatN(char* buf, size_t N, const char* fmt, Args... args) { You may want to attach the gcc format attribute (http://gcc.gnu.org/onlinedocs/gcc-4.8.1/gcc/Function-Attributes.html#index-Wf...) so the compiler can catch mistakes in the format string. https://codereview.chromium.org/18656004/diff/69001/base/debug/format.h#newco... base/debug/format.h:209: ARRAYSIZE_UNSAFE(arg_array)); arraysize(arg_array) should work, since internal::Arg has linkage. https://codereview.chromium.org/18656004/diff/69001/base/debug/format_unittes... File base/debug/format_unittest.cc (right): https://codereview.chromium.org/18656004/diff/69001/base/debug/format_unittes... base/debug/format_unittest.cc:197: Weirdly-located blank line. https://codereview.chromium.org/18656004/diff/69001/base/debug/format_unittes... base/debug/format_unittest.cc:421: // Decimals %d I think you're missing tests for truncated numbers. (You have truncated padding, but that's a different code path.)
Thanks for taking on this daunting task, async-signal safety is always regressing and a safe snprintf has been sorely needed in numerous places. I skimmed through it quickly, I'll need to spend a bit more time later. Some high level remarks: - I'm not sure "Format" is a great name, nor where this should belong in base/ (maybe a new posix/async-safe directory, or within a SignalSafe class ?). We should hold-off for OWNERS to chime in on that and not make any change. - Please split the unrelated changes as much as possible (for instance the use of this in sandbox/). - I'm not sure it's worth the pain of dealing with MSVC or in supporting non-POSIX in general. (Again we should hold for owner input). - Please document what your functions do (on top of them). - Make sure you have tests "at the exact boundaries". This will allow ASAN to detect any off-by-one issue. https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.cc File base/debug/format.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.cc:14: // As our contract promises that Format() can be called from any crazy nit: remove crazy https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.cc:34: #define RAW_DCHECK(x) do { if (x) { } } while (0) Do you want to just add this to base/logging.h ? It's about time we have a RAW_DCHECK. Also DCHECK is not *exactly* CHECK if !NDEBUG, we compile certain release builds with DCHECKs enabled to improve coverage. https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.cc:64: Please document this function and its |parameters|. https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.cc:65: inline bool IncrementCount(size_t* count, size_t inc = 1) { Please, avoid default parameters. Just have a IncrementCountGeneric() to implement the general case. https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.cc:81: inline bool Out(char* buf, size_t sz, size_t* count, char ch) { Please document this function and its |parameters| https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.cc:82: if (*count + 1 < sz) { if (sz >= 1 && count < sz - 1) https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.cc:91: inline void Pad(char* buf, size_t sz, size_t* count, char pad, size_t padding, Please document this function and its |parameters| https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.cc:196: goto cannot_write_anything_but_nul; Any way to split this to a subfunction ? https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.h File base/debug/format.h (right): https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.h:26: // Define ssize_t inside of our namespace. The comment is a bit misleading since this won't be scoped to a namespace. This probably belongs in base/basictypes.h ? https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.h:38: // be used in favor of FormatN() s/in favor/instead/ ? https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.h:67: // does not support at this time. If an actual user shows up, I would not be Could you try to phrase without the first person ? https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.h:104: // char buf[sz]; I'm a bit uneasy with recommending such a pattern. Most stack overflows are harmless for security, but it may not always be the case (at least it allows to DoS). This should at least state that "err" has to be trusted input. Recognizing trusted input is much harder than it seems most of the time, so I'm worried about this pattern. https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.h:152: const enum { INT, UINT, STRING, POINTER } type_; Typedefs and enum should be first, please look at the "Declaration order" in the style guide. https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/format.... base/debug/format.h:183: // compilers as soon as we have fully switched to C++11 Nit: final dot. https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/stack_t... File base/debug/stack_trace_unittest.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/89017/base/debug/stack_t... base/debug/stack_trace_unittest.cc:156: std::string itoa_r(intptr_t i, size_t sz, int base, size_t padding) { I wonder if you shouldn't expose itoa_r() and just leave most of this untouched.
Sending a new batch of remarks. Please, be extremely precise with the input and output constrains of helper functions such as "Out". Getting around C and C++ int overflows is incredibly tedious to write and review, so precision and comments are key. Both should be DCHECK-ed. And as remarked inline, the constrains should be a tad more dynamic: x < MAX_SSIZE_T is almost impossible to test for in practice. So, as suggested, make the max buffer size supported a global const, so that we can at least manually test for small values. If we could figure out a way that is not too intrusive for tests to set it dynamically, it would be awesome. https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format.cc File base/debug/format.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:34: #define RAW_DCHECK RAW_CHECK I would love a real RAW_DCHECK in logging.h. If what you want is a debug-mode-only thing, please call it something else. https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:68: // Returns "false", iff an overflow was detected. The documentation here isn't correct. You need to explain that count will be preventing to go over a ssize_t size, not a size_t. Explain that it's the caller responsibility for count and inc to be within a positive ssize_t range. https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:75: RAW_DCHECK((size_t)inc <= (size_t)std::numeric_limits<ssize_t>::max()); The first cast (of inc) isn't necessary. The second cast should normally be in C++, but I die a little inside every time I write these verbose lines. C++11 would allow you to have it as a global const, but we can't do that with C++03 How about: inline size_t GetMaxSSizeT() { return std::numeric_limits<ssize_t>::max(); } Since you seem to use this pattern a lot in this file, for good reason. An alternative is to update basictypes.h (look for "kint64max") and add a kssizetmax in there. But even better: it would be nice for this value to be tune-able, so that we could actually test for the code correctness in these edge cases. (So GetMaxSSizeT() would be GetMaxBuferSize() or something along these lines). Ideally we could tune this value dynamically in tests, but I'm not sure what the best route would be. A FormatImpl() that takes an extra size_t parameter for max buffer sizes would work, but it's a bit tedious to change everything. A static mutator / accessor would work, but it would also prevent inlining of the max buffer size value. Maybe the best compromise is an inline GetMaxBuferSize() function and we can just change it manually to test the code. https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:90: // Emits one |ch| character into the |buf| buffer of size |sz| and updates "updates the count" is not clear. "the count" means nothing in this context. Just say "increment count" ? https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:95: IncrementCountByOne(count); Shouldn't this be if (IncrementCountByOne(count)) XXX ? Why is this ok to not always increment count? https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:136: int base, size_t padding, char pad) { Style: don't mix input and outputs. Normally output should be at the end per our style guide. In this case, since a lot of the function have semantics similar to standard C, memcpy / snprintf etc, having outputs first is acceptable. But stick to one style! https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:229: *ptr++ = (upcase ? "0123456789ABCDEF" : "0123456789abcdef") I would define the base strings as static const char[] around the top of the file. https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:230: [num%base+minint]; Nit: X % Y + Z https://chromiumcodereview.appspot.com/18656004/diff/116001/base/debug/format... base/debug/format.cc:301: if (padding > max_padding/10 || style "X / Y"
I know I haven't addressed all of your comments just yet. But the refactoring should allow you to make progress on the code review. And it addresses your general concerns about ensuring we never write past the buffer boundaries. I hope, this CL makes it easier to reason about correctness of the code. Functionality should be mostly unchanged. The one difference is that I now guarantee the return code is never bigger than SSIZE_MAX-1. This allows callers to unconditionally add one to the return code (for storing the NUL) without triggering an integer overflow. This seems a friendlier API. We now have the ability to fake a smaller value of std::numeric_limits<ssize_t>::max(). I need to update the unittests to actually take advantage of this ability.
PTAL I think I now addressed all your initial comments, added the missing unittest, and cleaned things up a little more.
Sending the comments halfway through, so that you can start while I finish the review. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format.cc File base/debug/format.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:71: #ifdef NDEBUG #if defined(NDEBUG) https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:98: #endif // defined(NDEBUG) https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:101: class Buffer { I'm happy for a non publicly visible class, in an implementation file to be defined in-place, but other reviewers may disagree. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:101: class Buffer { Nit: no indent in a namespace. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:102: public: Nit: public is one space indent from "class Buffer". https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:104: // has |size| bytes of writable storage. Document clearly that size == 0 would lead to undefined behavior. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:117: DEBUG_CHECK(size > 0); Just make it a real RAW_CHECK. If someone passes size 0, we'd have memory corruption, so we need to crash cleanly in all cases. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:117: DEBUG_CHECK(size > 0); I would make this a real check. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:122: // The caller guaranteed that there was enough space to store a trailing The constructor guaranteed that... https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:138: // Return the number of bytes that have been emitted to |buffer_|. This This comment reads as-if you had a buffer overflow and didn't care. You have to say "The number of bytes that *would* have been emitted if enough space had been available", or something similar. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:150: // N.B. |count_| increases even if no characters have been written. This is Don't forget to explain how count_ work, where count_ is declared. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:243: char* buffer_; Add comments here. It's really important to explain the difference between size_ and count_ notably. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:246: }; DISALLOW_COPY_AND_ASSIGN() https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:248: Add some documentation, especially say what |i| is. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:268: if (i == std::numeric_limits<int64_t>::min()) { typeof(i) instead of int64_t ? https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:276: } else style: add {} https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:277: num = i; This is doing some dark magic. This has to be: 1. well documented when the function is declared. 2. documented better here with an explicit static_cast. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:298: } else style: add {}
https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format.cc File base/debug/format.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:233: // Return the current insertion point into the buffer. This is typically Please, define "insertion point" better. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:340: if (!num && started) Nit: braces are mandatory for multi-line if statements. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:361: // beyond this limit and we can compute the result arithmetically, as You forgot a verb in the first proposition of this sentence. This whole comment is a bit hard to read.
https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format.cc File base/debug/format.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format.cc:9: #include "base/debug/format.h" Please, have this as the first include as per style-guide (It is the interface to your implementation). https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... File base/debug/format_unittest.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format_unittest.cc:18: bool IsDeathTestAllowed() { base_unittests is pretty broken, death tests are broken as well. This shouldn't be fixed in-line here though. We should fix base_unittests, but that's a different endeavor. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format_unittest.cc:364: char *tmp = new char[sz+2]; Use a scoped_ptr here! https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format_unittest.cc:413: static_cast<long long>(std::numeric_limits<intptr_t>::min()), nit: indent https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format_unittest.cc:430: delete[] tmp; Use a scoped_ptr to not risk leaking memory if something uses ASSERT or somehow returns early. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format_unittest.cc:476: internal::SetFormatSSizeMax(std::numeric_limits<ssize_t>::max()); Please, use a scoper. Either create your own ScopedFormatSSizeMaxSetter() or use base::ScopedClosureRunner. https://chromiumcodereview.appspot.com/18656004/diff/157001/base/debug/format... base/debug/format_unittest.cc:726: Please, add a few ASAN-friendly tests. Have a buf of size X and use it to output strings of exactly size X.
I don't have time to go through all the comments one-by-one right now. But I think I addressed everything. Will talk to you tomorrow. I just uploaded a new version, so that we can verify which tests fail, if we enable death tests. TODO(markus): 1) Go through comments and mark everything that changed as "DONE". 2) Add myself to OWNERS for these files.
https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc File base/debug/format.cc (right): https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:9: #include "base/debug/format.h" On 2013/08/12 23:03:40, Julien Tinnes wrote: > Please, have this as the first include as per style-guide (It is the interface > to your implementation). Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:71: #ifdef NDEBUG On 2013/08/12 22:20:50, Julien Tinnes wrote: > #if defined(NDEBUG) Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:98: #endif On 2013/08/12 22:20:50, Julien Tinnes wrote: > // defined(NDEBUG) Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:101: class Buffer { On 2013/08/12 22:20:50, Julien Tinnes wrote: > Nit: no indent in a namespace. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:102: public: On 2013/08/12 22:20:50, Julien Tinnes wrote: > Nit: public is one space indent from "class Buffer". Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:104: // has |size| bytes of writable storage. On 2013/08/12 22:20:50, Julien Tinnes wrote: > Document clearly that size == 0 would lead to undefined behavior. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:117: DEBUG_CHECK(size > 0); On 2013/08/12 22:20:50, Julien Tinnes wrote: > Just make it a real RAW_CHECK. If someone passes size 0, we'd have memory > corruption, so we need to crash cleanly in all cases. As discussed earlier, I don't want to change this. We have to make sure that for production builds, the code is completely "pure". There should be no chance of any system calls leaking into the code -- even for error paths. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:138: // Return the number of bytes that have been emitted to |buffer_|. This On 2013/08/12 22:20:50, Julien Tinnes wrote: > This comment reads as-if you had a buffer overflow and didn't care. > > You have to say "The number of bytes that *would* have been emitted if enough > space had been available", or something similar. Done. Let me know, if this makes more sense. It's not always easy for me to tell whether my comments are clear to an outside observer. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:150: // N.B. |count_| increases even if no characters have been written. This is On 2013/08/12 22:20:50, Julien Tinnes wrote: > Don't forget to explain how count_ work, where count_ is declared. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:233: // Return the current insertion point into the buffer. This is typically On 2013/08/12 22:49:40, Julien Tinnes wrote: > Please, define "insertion point" better. Let me know if this is easier to understand. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:243: char* buffer_; On 2013/08/12 22:20:50, Julien Tinnes wrote: > Add comments here. It's really important to explain the difference between size_ > and count_ notably. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:246: }; On 2013/08/12 22:20:50, Julien Tinnes wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:248: On 2013/08/12 22:20:50, Julien Tinnes wrote: > Add some documentation, especially say what |i| is. All of that is documented inside of the class declaration. I believe that's where our style guide wants the documentation. Please let me know if you prefer me to change that. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:268: if (i == std::numeric_limits<int64_t>::min()) { On 2013/08/12 22:20:50, Julien Tinnes wrote: > typeof(i) instead of int64_t ? typeof() isn't part of the C++ standard -- unfortunately. decltype() might work, but I don't really want to check whether it is available on all of our target platforms. This doesn't seem sufficiently important here. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:276: } else On 2013/08/12 22:20:50, Julien Tinnes wrote: > style: add {} Done. In fact, I added all braces back, since you pointed out that the style guide actually allows me to do so. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:277: num = i; On 2013/08/12 22:20:50, Julien Tinnes wrote: > This is doing some dark magic. This has to be: > > 1. well documented when the function is declared. > 2. documented better here with an explicit static_cast. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:298: } else On 2013/08/12 22:20:50, Julien Tinnes wrote: > style: add {} Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:340: if (!num && started) On 2013/08/12 22:49:40, Julien Tinnes wrote: > Nit: braces are mandatory for multi-line if statements. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format.cc#new... base/debug/format.cc:361: // beyond this limit and we can compute the result arithmetically, as On 2013/08/12 22:49:40, Julien Tinnes wrote: > You forgot a verb in the first proposition of this sentence. > > This whole comment is a bit hard to read. I had trouble wording this comment. Let me know if this is a little easier to parse; if not, I'll just keep trying. https://codereview.chromium.org/18656004/diff/157001/base/debug/format_unitte... File base/debug/format_unittest.cc (right): https://codereview.chromium.org/18656004/diff/157001/base/debug/format_unitte... base/debug/format_unittest.cc:18: bool IsDeathTestAllowed() { On 2013/08/12 23:03:40, Julien Tinnes wrote: > base_unittests is pretty broken, death tests are broken as well. > > This shouldn't be fixed in-line here though. > > We should fix base_unittests, but that's a different endeavor. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format_unitte... base/debug/format_unittest.cc:364: char *tmp = new char[sz+2]; On 2013/08/12 23:03:40, Julien Tinnes wrote: > Use a scoped_ptr here! Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format_unitte... base/debug/format_unittest.cc:413: static_cast<long long>(std::numeric_limits<intptr_t>::min()), On 2013/08/12 23:03:40, Julien Tinnes wrote: > nit: indent Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format_unitte... base/debug/format_unittest.cc:430: delete[] tmp; On 2013/08/12 23:03:40, Julien Tinnes wrote: > Use a scoped_ptr to not risk leaking memory if something uses ASSERT or somehow > returns early. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format_unitte... base/debug/format_unittest.cc:476: internal::SetFormatSSizeMax(std::numeric_limits<ssize_t>::max()); On 2013/08/12 23:03:40, Julien Tinnes wrote: > Please, use a scoper. Either create your own ScopedFormatSSizeMaxSetter() or use > base::ScopedClosureRunner. Done. https://codereview.chromium.org/18656004/diff/157001/base/debug/format_unitte... base/debug/format_unittest.cc:726: On 2013/08/12 23:03:40, Julien Tinnes wrote: > Please, add a few ASAN-friendly tests. > > Have a buf of size X and use it to output strings of exactly size X. Done. Added ASAN support to PrintLongString(). That gives us the most test coverage, as it tests on all boundaries.
Sorry, I've still not gotten to it. I'll take a look tomorrow!
lgtm with a few remaining nits to address. I think this is ready for owners review. I would really like if: 1. This could be in a non debug/ directory. We need to use this all over the place in Chrome. Maybe base/async_safe. "Async safe" is typically a Posix term, but it could in a way apply on windows, for code (like the sandbox code) that would be called from deep hooks where locks etc could be held. 2. I'm not entirely sold on the Format() magic instead of FormatN. (I kind of like having explicit size, I would be happy with size being redundant and the C++ magic to just result in a check that the given size is less than the implicit size). https://chromiumcodereview.appspot.com/18656004/diff/175001/base/debug/format.cc File base/debug/format.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/175001/base/debug/format... base/debug/format.cc:125: // The caller guaranteed that there was enough space to store a trailing // The constructor guaranteed that... (or rephrase in another way). https://chromiumcodereview.appspot.com/18656004/diff/175001/base/debug/format... base/debug/format.cc:303: num = static_cast<uint64_t>(i); Please, document this cast. Explaining that the sign information is in |sign| etc. https://chromiumcodereview.appspot.com/18656004/diff/175001/base/debug/format... File base/debug/format_unittest.cc (right): https://chromiumcodereview.appspot.com/18656004/diff/175001/base/debug/format... base/debug/format_unittest.cc:728: } Do you mind adding a dead simple test that would catch off-by ones with ASAN ? I've seen that your more complicated test was amended for this, but it would made certain bug immediately obvious. something like: const char kTestString[] = "This is a test"; scoped_ptr.... (new ...[sizeof(kTestString)]); Format(buf, %s, kTestString); EXPECT_EQ().. It's redundant, but it would go a long way making a simple test fail on off-by-one. Other format characters would be appreciated.
Will, This is a type-safe and -- more importantly -- async-signal-safe implementation of snprintf(). We need it for all sorts of code that runs in restricted environments (e.g. the sandbox). In fact, it is safer than just async-signal-safe. It is guaranteed to not make any library or system calls whatsoever. Jeffrey and Julien have given a lot of useful feedback on this code, and they both seem happy with it now. Can you take a look as an OWNER of "base/" and either approve or suggest what I need to still do. The one outstanding issue is the final location for this code. It is part of a whole class of code that we should be writing going forward. For the time being, I put it in "base/debug/", as it is needed for debug-ish type of code. But I don't feel particularly wed to this directory location. Maybe, Julien is right, and we need a new directory? "base/async-safe/" sounds OK if nothing better is suggested. But it is a little confusing, as this code could very well be used on Windows, which doesn't even have the notion of async-signal-safety. Something like "base/pure/" would precisely spell out what it does, but might be too restrictive for some future code (which might want to make system calls) and is also hard for others to figure out what "pure" means. Similarly, "base/restricted/" is probably too confusing to be useful. Markus
I'm favorably inclined to this changelist. I need to examine the API in detail, and decide on a code location, but I'm in transit returning back to SF late Thursday. I'll see if I can review sometime when I'm at an airport. What do you think about putting this in base/strings? Naming is always hard, but what about something like base::SafeStringPrintf(), as we already have base::StringPrintf()? What are your thoughts along this vein?
I think this really depends on what we expect our long-term approach is going to be. If this remains essentially the only async-safe function; or if the only thing we add is a replacement for sscanf(), then "base/strings/" is probably a really good fit. On the other hand, if we find we need to add a bunch of other things (e.g. some sort of replacement for malloc()), then a different directory might make more sense. I'll confer with Julien what he thinks. For the immediate future, the "snprintf()" replacement was the main problem that we kept running into over and over again. So, maybe, that's really all we need right now.
On 2013/08/14 18:35:23, Markus (顧孟勤) wrote: > I think this really depends on what we expect our long-term approach > is going to be. If this remains essentially the only async-safe > function; or if the only thing we add is a replacement for sscanf(), > then "base/strings/" is probably a really good fit. > > On the other hand, if we find we need to add a bunch of other things > (e.g. some sort of replacement for malloc()), then a different > directory might make more sense. > > I'll confer with Julien what he thinks. For the immediate future, > the "snprintf()" replacement was the main problem that we kept running > into over and over again. So, maybe, that's really all we need right > now. Two others that we need are: - An async-signal safe light version of Pickle. S omething along the line of a vector that can be read (but not written to) in an async-signal safe manner. Perhaps these should just add pickle_async.h and something in containers/ respectively, but there could be advantages to a single directory, especially if we build helpers for unit tests (for instance a helper that will install a always-crash memory allocator). I think I'd be happy if this started in strings/ and if we tried to regroup thing once it clearly makes sense.
I moved and renamed the classes. Let me know, if this makes more sense. I am not super happy with the fact that safe_string_printf.cc could confuse users into thinking we use std::string. But that's a small nitpick. I named the methods SafeSPrintf() and SafeSNPrintf() to make it clear that we are modeling the well-known C API.
I didn't have time to review everything, but I did a first pass. I suspect I won't do a detailed review and will generally trust that jln&jyasskin handled all that. I'm happy with the location and happy to move it to a new location if that makes sense later on. If we're going to call it SafeSPrintf(), then perhaps we should name this safe_sprintf.cc? https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... File base/strings/safe_string_printf.cc (right): https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.cc:65: const size_t kSSizeMaxConst = ((size_t)(ssize_t)-1) >> 1; No indentation needed https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.cc:71: #if defined(NDEBUG) Why don't these preprocessor blocks just move into the above anonymous namespace? https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.cc:426: ssize_t internal::SafeSNPrintf(char* buf, size_t sz, const char* fmt, Hah, news to me that this syntax of namespace::fn works. If there's no good reason not to, do you mind just doing the normal namespace internal { ... } instead, just so no one else gets surprised by this syntax? Minor preference. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... File base/strings/safe_string_printf.h (right): https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.h:5: // Author: markus@chromium.org Is this a google3 holdover? We don't really do this in Chromium code. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.h:43: // requirements than just async-signal-safety. Is there a reason you don't say what you did in email: "It is guaranteed to not make any library or system calls whatsoever."? I'm not sure this last clause of "stricter requirements than just async-signal-safety" is useful, since without specifying what requirements it satisfies, I don't think you can know to use this code. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.h:127: // Helpers that use C++ overloading, templates, and specializations to deduce Google C++ style does not indent code within namespaces. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.h:158: int64_t i_; No trailing underscores. Please see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... which says "Data members in structs should be named like regular variables without the trailing underscores that data members in classes have." https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... File base/strings/safe_string_printf_unittest.cc (right): https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf_unittest.cc:14: #include "base/strings/safe_string_printf.h" Goes first for unittests too. C++ Style guide includes an exmaple of this. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf_unittest.cc:65: memset(ref, 'X', sizeof(buf)); arraysize instead perhaps?
I didn't have time to review everything, but I did a first pass. I suspect I won't do a detailed review and will generally trust that jln&jyasskin handled all that. I'm happy with the location and happy to move it to a new location if that makes sense later on. If we're going to call it SafeSPrintf(), then perhaps we should name this safe_sprintf.cc?
I think I addressed all your comments, but please let me know, if there is anything else you want me to edit. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... File base/strings/safe_string_printf.cc (right): https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.cc:65: const size_t kSSizeMaxConst = ((size_t)(ssize_t)-1) >> 1; On 2013/08/15 07:36:18, willchan wrote: > No indentation needed Done. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.cc:71: #if defined(NDEBUG) On 2013/08/15 07:36:18, willchan wrote: > Why don't these preprocessor blocks just move into the above anonymous > namespace? The #else clause has blocks that need to be in the anonymous namespace, and blocks that need to be in the "internal" namespace. I didn't see a cleaner way to express this with preprocessor conditionals. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.cc:426: ssize_t internal::SafeSNPrintf(char* buf, size_t sz, const char* fmt, On 2013/08/15 07:36:18, willchan wrote: > Hah, news to me that this syntax of namespace::fn works. Done. It hadn't even occurred to me that this was unusual. But I am happy to change it, if that makes it easier to read. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... File base/strings/safe_string_printf.h (right): https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.h:5: // Author: markus@chromium.org On 2013/08/15 07:36:18, willchan wrote: > Is this a google3 holdover? We don't really do this in Chromium code. Done. I had originally intended to only implement a relatively small subset of sprintf() and encourage users to contact me, if they needed more features. This was an attempt to reduce code complexity for potentially security-sensitive code. But after I tried switching some existing code over to using these new SafeSPrintf() functions, I discovered that a much larger set of format characters needs to be supported from day one. So, now, there really isn't a good reason to ask users to directly contact me. I just implemented most of the common features. Thanks for catching this. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.h:43: // requirements than just async-signal-safety. On 2013/08/15 07:36:18, willchan wrote: > Is there a reason you don't say what you did in email. Done. Does this sound better now? https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.h:127: // Helpers that use C++ overloading, templates, and specializations to deduce On 2013/08/15 07:36:18, willchan wrote: > Google C++ style does not indent code within namespaces. Done. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf.h:158: int64_t i_; On 2013/08/15 07:36:18, willchan wrote: > No trailing underscores. Please see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... > which says "Data members in structs should be named like regular variables > without the trailing underscores that data members in classes have." Done. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... File base/strings/safe_string_printf_unittest.cc (right): https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf_unittest.cc:14: #include "base/strings/safe_string_printf.h" On 2013/08/15 07:36:18, willchan wrote: > Goes first for unittests too. C++ Style guide includes an exmaple of this. Done. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf_unittest.cc:65: memset(ref, 'X', sizeof(buf)); On 2013/08/15 07:36:18, willchan wrote: > arraysize instead perhaps? Done. Not sure this is better, though. It now requires "sizeof(char)*arraysize(buf)". But I really don't feel strongly. If you think the "arraysize()" contributes to more clearly expressing the intent of the code, then I am happy making this change. I didn't change the call to "memcpy()" though, as that seems to be a common pattern for how to use "memcpy()". Let me know, if you disagree.
drive by. https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprintf.h File base/strings/safe_sprintf.h (right): https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:32: // SafeSPrintf() is a type-safe and async-signal-safe version of snprintf(). Can we be more specific (or more general? dunno) with the documentation? I mean, async-signal-safe, while a very specific concept, is also a POSIX concept so it would mean that this code should live on a _posix file, etc. Maybe something on the lines of being completely thread and exception safe? Or something about CRT dependency? https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:119: // size_t sz = kInitialSize; nit: I know this is just an example, but do you mind using a regular variable name? https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:184: BASE_EXPORT void SetSafeSPrintfSSizeMax(size_t max); If these are use only for tests they should be named FooForTest. Why debug only? Having tests do the same on debug and release is a good thing (but I have not looked at the tests so there may be a good reason there to do something different for debug builds).
Drive-by code reviews are always appreciated :-) https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprintf.h File base/strings/safe_sprintf.h (right): https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:32: // SafeSPrintf() is a type-safe and async-signal-safe version of snprintf(). On 2013/08/15 19:57:57, rvargas wrote: > Can we be more specific (or more general? dunno) with the documentation? This is just the one-line summary. If you keep reading two more paragraphs, I go into more detail. Is that sufficient? Or do you prefer I write this differently. I am certainly happy to take input, as it is always hard to write comments that people who are unfamiliar with the code appreciate. As a best case, I'd love if you gave the exact wording that you think would help you. But I am happy to work that out myself, if you give more details on why the one-line summary and following paragraphs don't work well. https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:119: // size_t sz = kInitialSize; On 2013/08/15 19:57:57, rvargas wrote: > nit: I know this is just an example, but do you mind using a regular variable > name? I renamed it to "size". Is that better? https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:184: BASE_EXPORT void SetSafeSPrintfSSizeMax(size_t max); On 2013/08/15 19:57:57, rvargas wrote: > If these are use only for tests they should be named FooForTest. > Why debug only? Having tests do the same on debug and release is a good thing > (but I have not looked at the tests so there may be a good reason there to do > something different for debug builds). Ah, learned something new :-) I had seen other code use the "internal" namespace without the "...ForTest". So, that's what I did as well. But of course, your suggestion makes even more sense. [ gory details and policy decisions ahead; feel free to skip ] As for the "debug-builds-only issue", you are touching a tricky topic. Our API guarantees that we never make any library or system calls and that we don't even touch "errno", as there are certainly users who would not be able to tolerate any of these. On the other hand, for debugging purposes, we would really like to use CHECK() (or more likely, RAWCHECK()). We finally compromised. In debug builds, we call RAWCHECK() if things look really wrong. For many developers, this is going to result in better diagnostics than just continuing. For some developers, it results in hangs or crashes, but for a pure debug build that's tolerable. For production builds, none of this is really an option. We just don't have a way to report exceptional error conditions, as that would violate the promise that we make in our API description. Instead, we specify a well-defined behavior for how we return "degraded" results. See the comments above on: not expanding arguments that have mismatching types. In practice, this is actually quite desirable behavior for many users. E.g. SafeSPrintf() might be used in the implementation of a crash reporting API. In that case, a degraded result is still useful at pointing to the root cause of the crash. But a crash report from inside SafeSPrintf() wouldn't be. The upshot of all of this is that our unittests need to test two slightly different error handling paths depending on whether they are build as a release or as a debug build.
thanks https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprintf.h File base/strings/safe_sprintf.h (right): https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:32: // SafeSPrintf() is a type-safe and async-signal-safe version of snprintf(). On 2013/08/15 20:31:20, Markus (顧孟勤) wrote: > On 2013/08/15 19:57:57, rvargas wrote: > > Can we be more specific (or more general? dunno) with the documentation? > > This is just the one-line summary. If you keep reading two more paragraphs, I go > into more detail. Is that sufficient? Or do you prefer I write this differently. > > I am certainly happy to take input, as it is always hard to write comments that > people who are unfamiliar with the code appreciate. > > As a best case, I'd love if you gave the exact wording that you think would help > you. But I am happy to work that out myself, if you give more details on why the > one-line summary and following paragraphs don't work well. Yeah, I like the explicit phrasing of "guaranteed to make no library or system calls", which is a superset of async-signal-safe. The thing that I don't like that much is that there are four mentions of async-ss and only one of the actual guarantees of the code. It's probably just nit picking, but I would be happier if the one line summary (the first thing a random user should see) contained that bit about being self contained code. I think it clears expectations right away (and self contained code is the characteristic that we want to preserve on future additions to this code, right?). How about SafeSPrintf() is a type-safe and completely self-contained version of snprintf(). Or maybe saying right away that doesn't make library or system calls? https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:119: // size_t sz = kInitialSize; On 2013/08/15 20:31:20, Markus (顧孟勤) wrote: > On 2013/08/15 19:57:57, rvargas wrote: > > nit: I know this is just an example, but do you mind using a regular variable > > name? > > I renamed it to "size". Is that better? Totally :) https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:184: BASE_EXPORT void SetSafeSPrintfSSizeMax(size_t max); On 2013/08/15 20:31:20, Markus (顧孟勤) wrote: > On 2013/08/15 19:57:57, rvargas wrote: > > If these are use only for tests they should be named FooForTest. > > Why debug only? Having tests do the same on debug and release is a good thing > > (but I have not looked at the tests so there may be a good reason there to do > > something different for debug builds). > > Ah, learned something new :-) I had seen other code use the "internal" namespace > without the "...ForTest". So, that's what I did as well. But of course, your > suggestion makes even more sense. > > > [ gory details and policy decisions ahead; feel free to skip ] > > As for the "debug-builds-only issue", you are touching a tricky topic. Our API > guarantees that we never make any library or system calls and that we don't even > touch "errno", as there are certainly users who would not be able to tolerate > any of these. On the other hand, for debugging purposes, we would really like to > use CHECK() (or more likely, RAWCHECK()). > > We finally compromised. In debug builds, we call RAWCHECK() if things look > really wrong. For many developers, this is going to result in better diagnostics > than just continuing. For some developers, it results in hangs or crashes, but > for a pure debug build that's tolerable. > > For production builds, none of this is really an option. We just don't have a > way to report exceptional error conditions, as that would violate the promise > that we make in our API description. Instead, we specify a well-defined behavior > for how we return "degraded" results. See the comments above on: not expanding > arguments that have mismatching types. > > In practice, this is actually quite desirable behavior for many users. E.g. > SafeSPrintf() might be used in the implementation of a crash reporting API. In > that case, a degraded result is still useful at pointing to the root cause of > the crash. But a crash report from inside SafeSPrintf() wouldn't be. > > The upshot of all of this is that our unittests need to test two slightly > different error handling paths depending on whether they are build as a release > or as a debug build. I suspected something along those lines... I saw the RAWCHECK code, and it makes sense to be more helpful in debug builds. So if the tests are making sure the debug code works that is fine. It was easier to ask :) Isn't it possible to double check the intended (release) behavior when simulating a lower limit? Maybe that doesn't make sense, and/or the test coverage is good enough as is... there is value in not having code compiled into the released product :) In any case, I'm happy with your decision.
https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprintf.h File base/strings/safe_sprintf.h (right): https://codereview.chromium.org/18656004/diff/184002/base/strings/safe_sprint... base/strings/safe_sprintf.h:32: // SafeSPrintf() is a type-safe and async-signal-safe version of snprintf(). On 2013/08/15 21:38:47, rvargas wrote: > SafeSPrintf() is a type-safe and completely self-contained version of > snprintf(). > > Or maybe saying right away that doesn't make library or system calls? Done. OK, I tweaked the comments a little more. I liked your suggestion about "completely self-contained". That certainly makes sense both for POSIX and for Windows programmers :-)
Will, just a quick reminder. I still need an LGTM from you (but I am out of the office for a few days, so take your time) Markus
My bad. I lost track of this CL. I'll try to remember to do it today. On Fri, Aug 30, 2013 at 11:31 AM, <markus@chromium.org> wrote: > Will, > > just a quick reminder. I still need an LGTM from you (but I am out of the > office > for a few days, so take your time) > > > Markus > > > https://codereview.chromium.**org/18656004/<https://codereview.chromium.org/1... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/08/30 16:27:54, willchan wrote: > My bad. I lost track of this CL. I'll try to remember to do it today. That would be fantastic! A lot of bug-fixing is waiting in this CL. Thanks!
LGTM, I only skimmed the .cc file. I trust that jln/jyasskin reviewed that in detail. https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... File base/strings/safe_string_printf_unittest.cc (right): https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf_unittest.cc:63: const char text[] = "hello world"; Perhaps static so it doesn't have to be copied into a local? https://codereview.chromium.org/18656004/diff/231001/base/strings/safe_string... base/strings/safe_string_printf_unittest.cc:65: memset(ref, 'X', sizeof(buf)); On 2013/08/15 08:20:46, Markus (顧孟勤) wrote: > On 2013/08/15 07:36:18, willchan wrote: > > arraysize instead perhaps? > > Done. > > Not sure this is better, though. It now requires "sizeof(char)*arraysize(buf)". > But I really don't feel strongly. If you think the "arraysize()" contributes to > more clearly expressing the intent of the code, then I am happy making this > change. > > I didn't change the call to "memcpy()" though, as that seems to be a common > pattern for how to use "memcpy()". Let me know, if you disagree. Sorry, it was a gut reaction to always do arraysize(). Upon re-reading it, I agree it's not clear which is better. https://codereview.chromium.org/18656004/diff/203001/base/strings/safe_sprint... File base/strings/safe_sprintf.cc (right): https://codereview.chromium.org/18656004/diff/203001/base/strings/safe_sprint... base/strings/safe_sprintf.cc:75: const size_t kSSizeMax = kSSizeMaxConst; As per style guide, don't indent within a namespace. Ditto in these other places in the file. https://codereview.chromium.org/18656004/diff/203001/base/strings/safe_sprintf.h File base/strings/safe_sprintf.h (right): https://codereview.chromium.org/18656004/diff/203001/base/strings/safe_sprint... base/strings/safe_sprintf.h:179: const Arg* args, size_t max_args); Re-indent this line.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markus@chromium.org/18656004/261001
Message was sent while issue was closed.
Change committed as 220746 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
