Closed Bug 1231581 Opened 4 years ago Closed 4 years ago

IonMonkey: MOZ_CRASH() in BacktrackingAllocator.cpp:LiveRange::toString() might be unreachable.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: wuwei, Assigned: wuwei)

References

Details

Attachments

(2 files, 3 obsolete files)

when i hit bug 1231575, i've noticed LiveRange::toString() was trying to check the return value of JS_snprintf():

> n = JS_snprintf(cursor, end - cursor, " %s", bundle()->allocation().toString());
> if (n < 0) MOZ_CRASH();
https://dxr.mozilla.org/mozilla-central/rev/319be5e7ce3061c7c16f24d750b6dacdbcac4c35/js/src/jit/BacktrackingAllocator.cpp#2233

since JS_snprintf returns uint32_t, if (n < 0) might be unreachable. (if JS_snprintf returns a value bigger than 2^31/2^63, then it could be.)

JS_snprintf() returns 0 if anything goes wrong. should we change this cond to
if (n == 0) MOZ_CRASH() ?
> JS_snprintf returns 0 if anything goes wrong
otherwise it returns the return value from JS_vsnprintf, which returns 'outlen' if anythings goes wrong.
On success JS_snprintf() returns the number of characters written not including the null terminator.  

On failure it's supposed to return the number of characters that would have been written had the buffer been large enough (not including the null terminator) but in fact it just returns the length of the buffer.

So the correct failure test is to check whether the return value is greater or equal to the size of the buffer.

If you fix this, can you also update the comment on the declaration of JS_snprintf() which incorrectly says it returns (uint32_t)-1 on failure?
(In reply to Jon Coppeard (:jonco) from comment #2)
> On success JS_snprintf() returns the number of characters written not
> including the null terminator.  
> 
> On failure it's supposed to return the number of characters that would have
> been written had the buffer been large enough (not including the null
> terminator) but in fact it just returns the length of the buffer.
> 
> So the correct failure test is to check whether the return value is greater
> or equal to the size of the buffer.
> 
> If you fix this, can you also update the comment on the declaration of
> JS_snprintf() which incorrectly says it returns (uint32_t)-1 on failure?

OK :)
Assignee: nobody → lazyparser
Blocks: 1231024
Depends on: 1231900
Attachment #8697527 - Flags: review?(bhackett1024)
Blocks: 1231899
Comment on attachment 8697527 [details] [diff] [review]
Part 1: check return value of JS_snprintf

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

This looks good, but while you're here can you change the MOZ_CRASH'es to 'return "<???>"' or similar?  Hopefully that will fix the IONFLAGS=regalloc crashes in bug 1231024.
Attachment #8697527 - Flags: review?(bhackett1024) → review+
(In reply to Brian Hackett (:bhackett) from comment #5)
> Comment on attachment 8697527 [details] [diff] [review]
> patch
> 
> Review of attachment 8697527 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but while you're here can you change the MOZ_CRASH'es to
> 'return "<???>"' or similar?  Hopefully that will fix the IONFLAGS=regalloc
> crashes in bug 1231024.

Sure. working on that bug :)
Attachment #8697757 - Flags: review?(bhackett1024)
Attachment #8697757 - Attachment is obsolete: true
Attachment #8697757 - Flags: review?(bhackett1024)
Attachment #8697758 - Flags: review?(bhackett1024)
Attachment #8697527 - Attachment description: patch → Part 1: check return value of JS_snprintf
Hit C4018 on win32 debug build. fixing.
Attached patch bug1231581.patchSplinter Review
fixed C4018 warning on win32 platform.
combined two small patches.
Attachment #8697527 - Attachment is obsolete: true
Attachment #8697758 - Attachment is obsolete: true
Attachment #8697758 - Flags: review?(bhackett1024)
Attachment #8698044 - Flags: review?(bhackett1024)
nit: remove a tail whitespace.
Attachment #8698046 - Flags: review?(bhackett1024)
Attachment #8698044 - Flags: review?(bhackett1024) → review+
Attachment #8698046 - Flags: review?(bhackett1024) → review+
Thank you for your review :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f0b2932c2ae3
https://hg.mozilla.org/mozilla-central/rev/624576c6558a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.