As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 991027 - JS_snprintf with "%hs" can fail with OOM
: JS_snprintf with "%hs" can fail with OOM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla31
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 912928 988953
  Show dependency treegraph
 
Reported: 2014-04-02 04:08 PDT by Jason Orendorff [:jorendorff]
Modified: 2014-04-26 19:06 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug-991027-jsprf-v1.patch (39.11 KB, patch)
2014-04-02 04:13 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description User image Jason Orendorff [:jorendorff] 2014-04-02 04:08:54 PDT
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.
Comment 1 User image Jason Orendorff [:jorendorff] 2014-04-02 04:13:18 PDT
Created attachment 8400558 [details] [diff] [review]
bug-991027-jsprf-v1.patch

Reformatted the comments. The main action of the patch is this generic_write stuff, and folding together cvt_s and cvt_ws.
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2014-04-07 14:54:05 PDT
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.
Comment 3 User image Jason Orendorff [:jorendorff] 2014-04-08 11:20:14 PDT
(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 User image Emanuel Hoogeveen [:ehoogeveen] 2014-04-09 05:46:31 PDT
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 :)
Comment 5 User image Jason Orendorff [:jorendorff] 2014-04-21 11:47:20 PDT
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...
Comment 6 User image Jason Orendorff [:jorendorff] 2014-04-21 12:41:51 PDT
Signs point to yes. https://tbpl.mozilla.org/?tree=Try&rev=601e80056755
Comment 7 User image Jason Orendorff [:jorendorff] 2014-04-21 13:36:07 PDT
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,
Comment 8 User image Jason Orendorff [:jorendorff] 2014-04-25 14:11:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eb5fefd5ca8
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2014-04-26 19:06:03 PDT
https://hg.mozilla.org/mozilla-central/rev/0eb5fefd5ca8

Note You need to log in before you can comment on or make changes to this bug.