720680 - Assertion failure: off >= 0 && (size_t) off < size, at js/src/jsopcode.cpp:923

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on mozilla-inbound revision 7026011a83df (options -m -n -a):


version(0);
eval("\
function TimeFromYear( y ) {}\
addTestCase( -2208988800000 );\
function addTestCase( t ) {\
  var start = TimeFromYear((addTestCase(addTestCase << t, 0)));\
    new TestCase( \
                  SECTION,\
                  '(new Date('+d+')).getUTCDay()',\
                  WeekDay((d)),\
                  (new Date(let ({ stop } = 'properties.length' )('/ab[c\\\n]/'))).getUTCDay() \
                );\
}\
");



This seems to be a regression of bug 688891, because the revision two steps earlier does not yet assert. An optimized build shows these Valgrind errors in addition:

==26961== Invalid read of size 2
==26961==    at 0x4C2910C: memcpy (mc_replace_strmem.c:628)
==26961==    by 0x4D0F81: js::Sprinter::put(char const*, unsigned long) (jsopcode.cpp:862)
==26961==    by 0x4DF0A6: DecompileDestructuringLHS(SprintStack*, unsigned char*, unsigned char*, int*, js::Vector<JSAtom*, 8ul, js::TempAllocPolicy>::Range*) (jsopcode.cpp:2078)
==26961==    by 0x4DF28D: DecompileDestructuring(SprintStack*, unsigned char*, unsigned char*, js::Vector<JSAtom*, 8ul, js::TempAllocPolicy>::Range*) (jsopcode.cpp:2222)
==26961==  Address 0x5e52e76 is 6 bytes after a block of size 128 free'd
==26961==    at 0x4C2710F: realloc (vg_replace_malloc.c:525)
==26961==    by 0x4D0E7A: js::Sprinter::put(char const*, unsigned long) (Utility.h:147)
==26961==    by 0x4DF0A6: DecompileDestructuringLHS(SprintStack*, unsigned char*, unsigned char*, int*, js::Vector<JSAtom*, 8ul, js::TempAllocPolicy>::Range*) (jsopcode.cpp:2078)
==26961==    by 0x4DF28D: DecompileDestructuring(SprintStack*, unsigned char*, unsigned char*, js::Vector<JSAtom*, 8ul, js::TempAllocPolicy>::Range*) (jsopcode.cpp:2222)


So there is likely a memory corruption involved here. sg:critical by these symptoms, and patch has already propagated to mozilla-central by now.

Comment 1

[Adam]

Attachment: 720680 #1

Fix patches fixes the crash/assertion failure, however when running the script I get:
720680.js:2: InternalError: too much recursion

I'm not sure if this is intentional or not

Also it may be interesting to find out if this problem could be triggered in the older code. The old code tends to "usually" have more than enough space after the end of offset to do crazy/insane things, the new code doesn't as it only allocates what it uses.

Comment 2

[Christian Holler (:decoder)]

(In reply to Adam from comment #1)
> however when running the script I get:
> 720680.js:2: InternalError: too much recursion
> 
> I'm not sure if this is intentional or not

It's not intentional, but not unexpected either. The fuzzer tests don't necessarily make sense (in a semantic way) because it's just code randomly shuffled around (at least for LangFuzz). On the non-failing revision I'm also seeing the recursion error.


> Also it may be interesting to find out if this problem could be triggered in
> the older code. The old code tends to "usually" have more than enough space
> after the end of offset to do crazy/insane things, the new code doesn't as
> it only allocates what it uses.

That would be really interesting to know for sure.

Comment 3

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

Comment on attachment 591121 [details] [diff] [review]
720680 #1

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

Ah, much better.

This test case actually hit another one of our assertions too (setting the offset beyond the buffer size):

$ ./js /tmp/bad.js 
Assertion failure: off >= 0 && (size_t) off < size, at /moz/mc-virgin/js/src/jsopcode.cpp:921
Aborted

(gdb) p size
$1 = 128
(gdb) p off
$2 = 129

I was wondering why we wouldn't have hit that one first, but we do hit it! Yay! :-)

Comment 4

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

Oh, I forgot, we should switch that to the checked js_memcpy now. I'll just add that little tweak before I push.

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/38a3bc6cc423

Adam, before we close this out, what are you thinking about the applicability of this bug to prior versions of the code?

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/38a3bc6cc423

Leaving open for comment 5.

Comment 7

[Adam]

The old code is okay, everything goes through Sprint() which is able to handle offset being > than size.

Comment 8

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/regress-bug720680.js.