602935 - TM: Always verify js_malloc(), js_calloc() return values.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: Fix malloc(), calloc() callers.

Grep and I noticed a few places in TraceMonkey where allocation functions are used without verifying that non-NULL has been returned. This fixes those locations:

ethogram_construct(), js_dtobasestr(), js_DumpOpMeters(), TraceRecorder::compile() [debug], TypedArrayTemplate::copyFromWithOverlap().

In the case of malloc failure in js_dtobasestr(), the code also dereferenced an uninitialized address. The fix checks for malloc failure and removes an unnecessary indentation level.

TypedArrayTemplate::copyFromWithOverlap(), and its caller ::copyFrom(), were made fallible.

js_ConcatStringsZ() was removed on approval from cdleary.

Comment 1

[Jason Orendorff [:jorendorff]]

Comment on attachment 481883 [details] [diff] [review]
Fix malloc(), calloc() callers.

In jstypedarray.cpp:
>@@ -927,7 +927,8 @@ class TypedArrayTemplate
>                 return false;
>             }
> 
>-            tarray->copyFrom(src, offset);
>+            if (!tarray->copyFrom(src, offset))
>+                return false;
>         } else if (arg0->wrappedObject(cx)->isArray()) {
>             jsuint len;
>             if (!js_GetLengthProperty(cx, arg0, &len))
>@@ -1007,7 +1008,8 @@ class TypedArrayTemplate
> 
>             if (!createBufferWithSizeAndCount(cx, sizeof(NativeType), tarray->length))
>                 return false;
>-            copyFrom(tarray);
>+            if (!copyFrom(tarray))
>+                return false;
>         } else if (other->getClass() == &ArrayBuffer::jsclass) {
>             ArrayBuffer *abuf = ArrayBuffer::fromJSObject(other);
> 

These two places, I think, need to js_ReportOutOfMemory(cx).

r=me with that.

Comment 2

[Nicholas Nethercote [inactive]]

Good catch!  But since prevention is better than cure, can we take a leaf out of posix_memalign()'s book and change the signature of our allocators to this?

  bool js_calloc(size_t bytes, void** p);

That makes it much harder to forget to check for failure.

Comment 3

[Brendan Eich [:brendan]]

>+            if (!copyFrom(tarray))
>+                return false;

Two places suggests making copyFrom take a leading cx and do its own reporting.

> That makes it much harder to forget to check for failure.

People can ignore a return value no matter what. I don't see why we should pay the cost of an out param here.

Rather, let's have a static analysis to check for allocator return values not null-checked. And separately, get to the infallible (small) alloc promised land, which will reduce the bug surface quite a bit.

/be

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
>
> And separately, get to the infallible (small) alloc promised
> land, which will reduce the bug surface quite a bit.

Oh, I thought that was never going to happen in js/.  How will it happen?  Where can I sign up?

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> (In reply to comment #3)
> >
> > And separately, get to the infallible (small) alloc promised
> > land, which will reduce the bug surface quite a bit.
> 
> Oh, I thought that was never going to happen in js/.

Why not?

> How will it happen? Where can I sign up?

It's part of the e10s plan, which will not be implemented fully (i.e., reliably) for Firefox 4 / Mozilla 2. See bug 599791 comment 19 and bug 521309 comment 11, among others. Cc'ing cjones.

/be

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> > 
> > Oh, I thought that was never going to happen in js/.
> 
> Why not?

Hmm... I found the old js-engine newsgroup thread titled "Out of memory (mallloc returns null) handling policy" which discussed the possibility of infallible malloc.  It's a hard thread to read, the issue is clearly complicated, and it appears my tiny pessimistic brain internally summarized that thread as "it'll never happen" :)  I'll pipe down now.

Comment 7

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/tracemonkey/rev/f97be04eea98

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #3)
> People can ignore a return value no matter what.

While this is certainly true, we could partially help ourselves here by adding __attribute__((warn_unused_result)) (macroized) to some of these functions (although not the ones that return a useful value like a maybe-NULL pointer).  With this, at least on gcc-using platforms, we'd get compiler warnings calling such functions without using their return values.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (In reply to comment #3)
> > People can ignore a return value no matter what.
> 
> While this is certainly true, we could partially help ourselves here by adding
> __attribute__((warn_unused_result)) (macroized) to some of these functions
> (although not the ones that return a useful value like a maybe-NULL pointer). 
> With this, at least on gcc-using platforms, we'd get compiler warnings calling
> such functions without using their return values.

Good idea -- file a bug?

/be

Comment 10

[Jeff Walden [:Waldo]]

Filed bug 605349 concerning comment 8.

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/f97be04eea98