809292 - realloc(0,0) may return NULL but not OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ginn Chen]

C99 has:
Implementation-defined behavior
— Whether the calloc, malloc, and realloc functions return a null pointer or a pointer to an allocated object when the size requested is zero.

POSIX has:
 If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() shall be returned. If there is not enough available memory, realloc() shall return a null pointer [CX] [Option Start]  and set errno to [ENOMEM].
http://pubs.opengroup.org/onlinepubs/009695399/functions/realloc.html


In jsscript.cpp, we have

990 if (compressedLength == 0) {
991     // Note ss->data.source might be NULL.
992     jschar *buf = static_cast<jschar *>(js_realloc(ss->data.source, nbytes));
993     if (!buf) {
994         if (ss->data.source) {
995             js_free(ss->data.source);
996             ss->data.source = NULL;
997         }
998         return false;
999     }
1000    ss->data.source = buf;
1001    PodCopy(ss->data.source, tok->chars, ss->length());
1002 } else {

So, if ss->data.source = NULL, nbytes = 0, buf can be NULL, but we are not OOM, because errno is not ENOMEM, return false will break the testcase.

Note: return true and setting with buf = NULL will break /js/src/jit-test/tests/basic/function-tosource-constructor.js.
mozilla-central/js/src/jit-test/tests/basic/function-tosource-constructor.js:14:0 Error: Assertion failed: got "function anonymous() {\n    [sourceless code]\n}", expected "function anonymous() {\n\n}"

Returning true and set buf to a valid address fixes the problem, however I don't know if it is the right approach.

Compiling js/src along doesn't have this problem.

Comment 1

[:Benjamin Peterson]

Attachment: consolidate all messing with source data allocation into one function, which gets it right

I should have done something like this to start with.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Benjamin's patch looks like a nice refactoring, but I don't think we should be adding special cases to deal with malloc and realloc of 0 bytes. We normally build with jemalloc, and it does what we need in these situations. On other platforms, it seems like we could use a shim around malloc that also does what we need.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 679245 [details] [diff] [review]
consolidate all messing with source data allocation into one function, which gets it right

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

The refactoring is a good thing.

Unlike billm I'm happy with handling the undefined behaviour here -- shimming malloc on non-jemalloc platforms sounds like a headache.  Benjamin, I'll let you decide which path to take.

::: js/src/jsscript.cpp
@@ +1068,5 @@
>      stop = true;
>  }
>  #endif /* JS_THREADSAFE */
>  
> +static unsigned char emptySource[] = "";

It would be helpful to have a comment somewhere explaining that NULL indicates something went wrong, and emptySource means that nothing went wrong but the source size was zero.

@@ +1276,5 @@
>  {
>      JS_ASSERT(ready());
>  
> +    // data is a union, but both members are pointers to allocated memory,
> +    // emptySource, or NULL, so just using compressed will work.

I find sentences like these much easier to read if you write field names as e.g. |data| and |compressed|.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Ginn, does this patch actually fix anything for you? As far as I can tell, we call malloc(0) all over the place in the engine and assume that NULL means OOM. Why aren't those a problem as well?

Comment 5

[Ginn Chen]

(In reply to Bill McCloskey (:billm) from comment #4)
> Ginn, does this patch actually fix anything for you? As far as I can tell,
> we call malloc(0) all over the place in the engine and assume that NULL
> means OOM. Why aren't those a problem as well?

Yes, it fixed the problem.

Actually I'm using an odd libc, which returns NULL for realloc(0,0) but a valid pointer for malloc(0).

I've another malloc library returns NULL for both realloc(0,0) and malloc(0).
I'll try it and let you know the results.

Comment 6

[Ginn Chen]

Tested on 2 systems.

1) malloc(0) returns 0; realloc(0,0) returns 0
With the patch, js/src 'make test' all passed.

2) malloc(0) returns 0; realloc(0,0) returns a magic pointer, which is considered as a NULL pointer if passed to free().

Without the patch, got OOM error.
With the patch, js/src 'make test' all passed.

Comment 7

[:Benjamin Peterson]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f116ae5e387

If we do end up making a malloc shim, the special casing here can just be ripped out.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f116ae5e387