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

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Created attachment 481883 [details] [diff] [review]
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.
Attachment #481883 - Flags: review?(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.
Attachment #481883 - Flags: review?(jorendorff) → review+
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.
>+            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
(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?
(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
(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.
(Reporter)

Comment 7

8 years ago
http://hg.mozilla.org/tracemonkey/rev/f97be04eea98
Whiteboard: fixed-in-tracemonkey

Comment 8

8 years ago
(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.
(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
Filed bug 605349 concerning comment 8.
http://hg.mozilla.org/mozilla-central/rev/f97be04eea98
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.