831040 - js_ReportAllocationOverflow should not be able to GC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Comment 2

[Brian Hackett [Laid off!]]

Attachment: add a few more AutoSuppressGCs

Some error reporting on malformed inputs when inflating/deflating UTF8 strings is also causing issues.

Comment 3

[Terrence Cole [: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.

Comment 4

[Terrence Cole [:terrence]]

On second thought, we only want to require suppression for OOM, so this is fine as is.

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f719315ea412

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f719315ea412