Closed
Bug 991027
Opened 10 years ago
Closed 10 years ago
JS_snprintf with "%hs" can fail with OOM
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: jorendorff, Assigned: jorendorff)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
39.11 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
We have this code in SPSprofiler::allocProfileString: if (hasAtom) ret = JS_snprintf(cstr, len + 1, "%hs (%s:%llu)", atom, filename, lineno); else ret = JS_snprintf(cstr, len + 1, "%s:%llu", filename, lineno); MOZ_ASSERT(ret == len, "Computed length should match actual length!"); Even though we have preallocated a buffer for cstr, JS_snprintf can fail because the code for handling "%hs" allocates another temporary buffer. After a little thought I decided to keep the assertion and make %hs not allocate memory. With that, this snprintf is infallible. Admittedly it feels dumb to be spending time in this code.
Assignee | ||
Comment 1•10 years ago
|
||
Reformatted the comments. The main action of the patch is this generic_write stuff, and folding together cvt_s and cvt_ws.
Assignee: general → jorendorff
Attachment #8400558 -
Flags: review?(jwalden+bmo)
Comment 2•10 years ago
|
||
Comment on attachment 8400558 [details] [diff] [review] bug-991027-jsprf-v1.patch Review of attachment 8400558 [details] [diff] [review]: ----------------------------------------------------------------- Very pig. Such lipstick. Wow. Not super-clear how much cleanup you actually want to do here if you're just fixing OOM, versus pushing it into another patch. Oh well. ::: js/src/jsprf.cpp @@ -35,5 @@ > #define VARARGS_ASSIGN(foo, bar) (foo) = (bar) > #endif > > -/* > -** WARNING: This code may *NOT* call JS_LOG (because JS_LOG calls it) Hah, this comment isn't even *wrong*. @@ +52,5 @@ > + */ > +struct NumArgState > +{ > + int type; // type of the current ap > + va_list ap; // point to the corresponding position on ap I would get rid of the name alignment here. It's just a nuisance that makes nearby changes harder to make, without enough readability gain. @@ +57,3 @@ > }; > > +#define NAS_DEFAULT_NUM 20 // default number of NumberedArgumentState array const size_t NAS_DEFAULT_NUM = 20; maybe. You'll have to change a |number| variable to size_t later on to avoid a comparison warning, but no further extent than that. @@ +74,1 @@ > #define TYPE_UNKNOWN 20 Could enum these up too, maybe. @@ +116,5 @@ > char space = ' '; > int rv; > > width -= srclen; > + if ((width > 0) && ((flags & FLAG_LEFT) == 0)) { // Right adjusting No parentheses around the first half, or around the whole of the second half. @@ +134,3 @@ > if (rv < 0) { > return rv; > } Extra braces. @@ +134,5 @@ > if (rv < 0) { > return rv; > } > > + if ((width > 0) && ((flags & FLAG_LEFT) != 0)) { // Left adjusting Same. @@ +139,5 @@ > while (--width >= 0) { > rv = (*ss->stuff)(ss, &space, 1); > if (rv < 0) { > return rv; > } Extra braces. @@ +236,5 @@ > } > return 0; > } > > +/* Convert a long into its printable form */ Full sentence, ergo period. @@ +271,4 @@ > return fill_n(ss, cvt, digits, width, prec, type, flags); > } > > +/* Convert a 64-bit integer into its printable form */ Period. @@ +275,4 @@ > static int cvt_ll(SprintfState *ss, int64_t num, int width, int prec, int radix, > int type, int flags, const char *hexp) > { > + // according to the man page this needs to happen Period, capitalize. @@ +279,3 @@ > if (prec == 0 && num == 0) { > return 0; > } No braces. @@ +372,3 @@ > > + // and away we go > + return fill2(ss, s, slen, width, flags); Mario is running through my head now. @@ +390,4 @@ > > > + // first pass: > + // detemine how many legal % I have got, then allocate space detemine, caps, period. @@ +427,2 @@ > return nullptr; > } No braces. @@ +428,5 @@ > } > > > + if (number > NAS_DEFAULT_NUM) { > + nas = (NumArgState *) js_malloc(number * sizeof(NumArgState)); Hum, don't we have something that does this multiplication for us? That doesn't require cx-> stuffs? Blah. @@ +443,5 @@ > } > > > + // second pass: > + // set nas[].type Caps and all. @@ +452,2 @@ > c = *p++; > + if (c == '%') continue; Might as well put the continues on new lines. @@ +462,5 @@ > *rv = -1; > break; > } > > + // nas[cn] starts from 0, and make sure nas[cn].type is not assigned Period. @@ +579,4 @@ > > + if (*rv < 0) { > + if (nas != nasArray) > + js_free (nas); No space after js_free. This manual memory handling gives me warm fuzzies. @@ +651,3 @@ > > + // build an argument array, IF the fmt is numbered argument > + // list style, to contain the Numbered Argument list pointers Caps, period. @@ +669,5 @@ > } > fmt0 = fmt - 1; > > + // Gobble up the % format string. Hopefully we have handled all > + // of the strange cases! Hopefully. @@ +690,5 @@ > c = *fmt++; > } > > + if (nas[i-1].type == TYPE_UNKNOWN) { > + if (nas && (nas != nasArray)) Extra parens seem unnecessary. @@ +856,2 @@ > i = fmt - dolPt; > + if (i < (int) sizeof(pattern)) { int() C++ cast? @@ +858,3 @@ > pattern[0] = '%'; > + js_memcpy(&pattern[1], dolPt, size_t(i)); > + rv = cvt_f(ss, u.d, pattern, &pattern[i+1]); Spaces around +. @@ +964,1 @@ > } Braces, extra parens. @@ +982,5 @@ > off = ss->cur - ss->base; > if (off + len >= ss->maxlen) { > /* Grow the buffer */ > newlen = ss->maxlen + ((len > 32) ? len : 32); > + newbase = (char *) js_realloc(ss->base, newlen); static_cast<>? @@ +1020,5 @@ > /* > + * Free memory allocated, for the caller, by JS_smprintf > + */ > +JS_PUBLIC_API(void) > +JS_smprintf_free(char *mem) Strange API to expose, versus telling people to JS_free or so. @@ +1052,2 @@ > { > + size_t limit = ss->maxlen - (ss->cur - ss->base); You might want mozilla::PointerRangeSize for this subtraction. @@ +1054,4 @@ > > if (len > limit) { > len = limit; > } Braces. @@ +1074,5 @@ > > JS_ASSERT(int32_t(outlen) > 0); > if (int32_t(outlen) <= 0) { > return 0; > } Braces. @@ +1083,5 @@ > return rv; > } > > +JS_PUBLIC_API(uint32_t) > +JS_vsnprintf(char *out, uint32_t outlen, const char *fmt, va_list ap) Doesn't outlen want to become size_t, same for n? @@ +1100,5 @@ > ss.maxlen = outlen; > (void) dosprintf(&ss, fmt, ap); > > /* If we added chars, and we didn't append a null, do it now. */ > + if ((ss.cur != ss.base) && (ss.cur[-1] != '\0')) Extra parens twice.
Attachment #8400558 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2) > Very pig. Such lipstick. Wow. Ha! I took all the cosmetic suggestions but deferred these more substantial changes: > @@ +74,1 @@ > > #define TYPE_UNKNOWN 20 > > Could enum these up too, maybe. > > +JS_smprintf_free(char *mem) > > Strange API to expose, versus telling people to JS_free or so. > > + size_t limit = ss->maxlen - (ss->cur - ss->base); > > You might want mozilla::PointerRangeSize for this subtraction. > > +JS_PUBLIC_API(uint32_t) > > +JS_vsnprintf(char *out, uint32_t outlen, const char *fmt, va_list ap) > > Doesn't outlen want to become size_t, same for n? Follow-up bugs if you think these are worth doing. Regarding PointerRangeSize, note that the idiom 'cur - base' is used in several other places too. Could be a method. Regarding JS_smprintf_free, if we're going to improve the API, RAII-ify it. Depending on usage patterns. (There are something like 15 callers!)
Comment 4•10 years ago
|
||
I switched this code over from StringBuffer to JS_snprintf to remove the JSContext requirement, but I had no idea that this case could lead to extra allocation! Thanks for fixing :)
Assignee | ||
Comment 5•10 years ago
|
||
Ah. Try server informs me that this patch burns Hf because I forgot to add this: diff --git a/js/src/devtools/rootAnalysis/annotations.js b/js/src/devtools/rootAnalysis/annotations.js --- a/js/src/devtools/rootAnalysis/annotations.js +++ b/js/src/devtools/rootAnalysis/annotations.js @@ -62,7 +62,7 @@ function indirectCallCannotGC(fullCaller var ignoreClasses = { "JSTracer" : true, "JSStringFinalizer" : true, - "SprintfStateStr" : true, + "SprintfState" : true, "JSLocaleCallbacks" : true, "JSC::ExecutableAllocator" : true, "PRIOMethods": true, Trying again...
Assignee | ||
Comment 6•10 years ago
|
||
Signs point to yes. https://tbpl.mozilla.org/?tree=Try&rev=601e80056755
Assignee | ||
Comment 7•10 years ago
|
||
Lol. That burns Hf too, because there are two more copies of this printf code elsewhere in the tree. So the correct patch is: diff --git a/js/src/devtools/rootAnalysis/annotations.js b/js/src/devtools/rootAnalysis/annotations.js --- a/js/src/devtools/rootAnalysis/annotations.js +++ b/js/src/devtools/rootAnalysis/annotations.js @@ -62,7 +62,8 @@ function indirectCallCannotGC(fullCaller var ignoreClasses = { "JSTracer" : true, "JSStringFinalizer" : true, "SprintfStateStr" : true, + "SprintfState" : true, "JSLocaleCallbacks" : true, "JSC::ExecutableAllocator" : true, "PRIOMethods": true,
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eb5fefd5ca8
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0eb5fefd5ca8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•