Closed Bug 991027 Opened 6 years ago Closed 6 years ago

JS_snprintf with "%hs" can fail with OOM

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
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)
Blocks: 912928, 988953
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+
(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!)
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 :)
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...
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,
https://hg.mozilla.org/mozilla-central/rev/0eb5fefd5ca8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.