Closed Bug 720680 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla12
Tracking Status
firefox9 --- unaffected
firefox10 --- unaffected
firefox11 --- unaffected
firefox12 + fixed
firefox-esr10 --- unaffected
status1.9.2 --- unaffected

People

(Reporter: decoder, Assigned: adam)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sg:critical] js-triage-done)

Attachments

(1 file)

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.
Attached patch 720680 #1Splinter Review
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.
Attachment #591121 - Flags: review?
(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 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! :-)
Attachment #591121 - Flags: review? → review+
Oh, I forgot, we should switch that to the checked js_memcpy now. I'll just add that little tweak before I push.
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?
Assignee: general → adam
Status: NEW → ASSIGNED
Whiteboard: [sg:critical] js-triage-needed → [sg:critical] js-triage-done
Target Milestone: --- → mozilla12
Keywords: regression
The old code is okay, everything goes through Sprint() which is able to handle offset being > than size.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Group: core-security
Status: RESOLVED → VERIFIED
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/regress-bug720680.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.