Closed Bug 831040 Opened 7 years ago Closed 7 years ago

js_ReportAllocationOverflow should not be able to GC

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bhackett1024, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

This function gets called in various allocator routines when the allocated space is too large.  Examples are JSContext::pod_malloc/pod_calloc.  Similar to js_ReportOutOfMemory, it should be impossible for this function to trigger a GC, as otherwise all sorts of unexpected GCs could occur.
Attached patch patch (obsolete) — Splinter Review
Attachment #702552 - Flags: review?(terrence)
Some error reporting on malformed inputs when inflating/deflating UTF8 strings is also causing issues.
Attachment #702552 - Attachment is obsolete: true
Attachment #702552 - Flags: review?(terrence)
Attachment #702892 - Flags: review?(terrence)
Comment on attachment 702892 [details] [diff] [review]
add a few more AutoSuppressGCs

Review of attachment 702892 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. This is pretty excellent, so lets go ahead and do it.

::: js/src/jsstr.cpp
@@ +3856,5 @@
>      if (srclen > dstlen) {
>          for (size_t i = 0; i < dstlen; i++)
>              dst[i] = (char) src[i];
>          if (maybecx) {
> +            AutoSuppressGC suppress(maybecx);

I had planned to eventually change the API to treat a too-short buffer as API misuse and crash appropriately, but given our timeframe, I think this is the better approach.

Please wrap this as js_ReportBufferTooSmall in jscntxt.cpp so that these don't get lost when I or someone else moves this all to vm/CharacterEncoding.cpp, where it belongs.

@@ +3929,5 @@
>                          JS_ReportErrorFlagsAndNumber(cx,
>                                                       JSREPORT_ERROR,
>                                                       js_GetErrorMessage, NULL,
>                                                       JSMSG_UTF8_CHAR_TOO_LARGE,
>                                                       buffer);

Likewise, wrap this as js_ReportUTF8CharTooLarge.
Attachment #702892 - Flags: review?(terrence) → review+
On second thought, we only want to require suppression for OOM, so this is fine as is.
https://hg.mozilla.org/mozilla-central/rev/f719315ea412
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.