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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wei Wu [:wuwei UTC+8]]

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() ?

Comment 1

[Wei Wu [:wuwei UTC+8]]

> JS_snprintf returns 0 if anything goes wrong
otherwise it returns the return value from JS_vsnprintf, which returns 'outlen' if anythings goes wrong.

Comment 2

[Jon Coppeard (:jonco)]

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?

Comment 3

[Wei Wu [:wuwei UTC+8]]

(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 :)

Comment 4

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 1: check return value of JS_snprintf

Comment 5

[Brian Hackett [Laid off!]]

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.

Comment 6

[Wei Wu [:wuwei UTC+8]]

(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 :)

Comment 7

[Wei Wu [:wuwei UTC+8]]

Attachment: part 2: replace MOZ_CRASH with a hint message

Comment 8

[Wei Wu [:wuwei UTC+8]]

Attachment: part 2: replace MOZ_CRASH with a hint message

Comment 9

[Wei Wu [:wuwei UTC+8]]

Hit C4018 on win32 debug build. fixing.

Comment 10

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1231581.patch

fixed C4018 warning on win32 platform.
combined two small patches.

Comment 11

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1231581nit.patch

nit: remove a tail whitespace.

Comment 12

[Wei Wu [:wuwei UTC+8]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c762021cebb5

Comment 13

[Wei Wu [:wuwei UTC+8]]

Thank you for your review :)

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b2932c2ae3
https://hg.mozilla.org/integration/mozilla-inbound/rev/624576c6558a

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0b2932c2ae3
https://hg.mozilla.org/mozilla-central/rev/624576c6558a