991027 - JS_snprintf with "%hs" can fail with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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

[Jason Orendorff [:jorendorff]]

Attachment: 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

[Jeff Walden [:Waldo]]

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

[Jason Orendorff [:jorendorff]]

(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

[Emanuel Hoogeveen [:ehoogeveen]]

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

[Jason Orendorff [:jorendorff]]

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

[Jason Orendorff [:jorendorff]]

Signs point to yes. https://tbpl.mozilla.org/?tree=Try&rev=601e80056755

Comment 7

[Jason Orendorff [:jorendorff]]

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

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0eb5fefd5ca8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0eb5fefd5ca8