1355046 - Assertion failure: ptrdiff_t(column) + colspan >= 0, at js/src/jsscript.cpp:3102

Status: RESOLVED FIXED

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 2a3ecdb7d1ea (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --ion-offthread-compile=off):

var localstr = "";
for (var i = 0; i < 0xFFFC; ++i)
  localstr += ('\f') + i + "; ";
var arg = "x";
var body = localstr + "for (var i = 0; i < 4; ++i) arr[i](x-1);";
(new Function(arg, body))(1000);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x08601271 in js::PCToLineNumber (startLine=<optimized out>, notes=0xf4786053 "\220\220\210\001\210\005\210\t\210\r\210\021\210\025\210\031\210\035\210!\210%\210)\210.\210\063\210\070\210=\210B\210G\210L\210Q\210V\210[\210`\210e\210j\210o\210t\210y\210~\210\200", code=0xf4786014 ">W", pc=0xf4786021 "\232", columnp=0xffffb798) at js/src/jsscript.cpp:3102
#0  0x08601271 in js::PCToLineNumber (startLine=<optimized out>, notes=0xf4786053 "\220\220\210\001\210\005\210\t\210\r\210\021\210\025\210\031\210\035\210!\210%\210)\210.\210\063\210\070\210=\210B\210G\210L\210Q\210V\210[\210`\210e\210j\210o\210t\210y\210~\210\200", code=0xf4786014 ">W", pc=0xf4786021 "\232", columnp=0xffffb798) at js/src/jsscript.cpp:3102
#1  0x086012c8 in js::PCToLineNumber (script=0xf55700f8, pc=0xf4786021 "\232", columnp=0xffffb798) at js/src/jsscript.cpp:3120
#2  0x088001c7 in js::FrameIter::computeLine (this=0xffffb570, column=0xffffb798) at js/src/vm/Stack.cpp:896
#3  0x085582f8 in PopulateReportBlame (cx=cx@entry=0xf791d000, report=report@entry=0xffffb78c) at js/src/jscntxt.cpp:314
#4  0x08570608 in js::ReportErrorNumberVA (cx=0xf791d000, cx@entry=0xf7900000, flags=flags@entry=0, callback=0x8549bc0 <js::GetErrorMessage(void*, unsigned int)>, callback@entry=0x39d37a00, userRef=0x0, errorNumber=1, ap=0xffffb820 "\224\200\225\367\364\377\321\bh\270\377\377{\200Y\bL\270\377\377", argumentsType=js::ArgumentsAreLatin1) at js/src/jscntxt.cpp:883
#5  0x08570915 in JS_ReportErrorNumberLatin1VA (cx=0x0, errorCallback=0x1, userRef=0xffffb820, errorNumber=1, ap=0xffffb820 "\224\200\225\367\364\377\321\bh\270\377\377{\200Y\bL\270\377\377") at js/src/jsapi.cpp:5784
#6  0x08570947 in JS_ReportErrorNumberLatin1 (cx=0xf791d000, errorCallback=0x8549bc0 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=1) at js/src/jsapi.cpp:5773
#7  0x08570c0f in js::ReportIsNotDefined (cx=0xf791d000, id=...) at js/src/jscntxt.cpp:951
#8  0x08570c98 in js::ReportIsNotDefined (cx=0xf791d000, name=...) at js/src/jscntxt.cpp:960
#9  0x081795a4 in js::FetchName<(js::GetNameMode)0> (cx=0xf791d000, receiver=..., holder=..., name=..., prop=..., vp=...) at js/src/vm/Interpreter-inl.h:187
#10 0x0816b84e in js::GetEnvironmentName<(js::GetNameMode)0> (vp=..., name=..., envChain=..., cx=<optimized out>) at js/src/vm/Interpreter-inl.h:257
#11 GetNameOperation (vp=..., pc=<optimized out>, fp=<optimized out>, cx=<optimized out>) at js/src/vm/Interpreter.cpp:218
#12 Interpret (cx=0xf791d000, state=...) at js/src/vm/Interpreter.cpp:3139
#13 0x0816f855 in js::RunScript (cx=0xf791d000, state=...) at js/src/vm/Interpreter.cpp:395
#14 0x0816fdcf in js::InternalCallOrConstruct (cx=0xf791d000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:473
#15 0x081700ac in InternalCall (cx=cx@entry=0xf791d000, args=...) at js/src/vm/Interpreter.cpp:500
#16 0x081701ff in js::CallFromStack (cx=0xf791d000, args=...) at js/src/vm/Interpreter.cpp:506
#17 0x08235b6f in js::jit::DoCallFallback (cx=0xf791d000, frame=0xffffc028, stub_=0xf53943a0, argc=1, vp=0xffffbfe8, res=...) at js/src/jit/BaselineIC.cpp:2353
#18 0x5105eb8c in ?? ()
#19 0xf53943a0 in ?? ()
#20 0x51057c66 in ?? ()
eax	0x0	0
ebx	0x7ffe5cf2	2147376370
ecx	0xf7da4864	-136689564
edx	0x0	0
esi	0xf47a3b24	-193316060
edi	0x0	0
ebp	0xffffb4f8	4294948088
esp	0xffffb4d0	4294948048
eip	0x8601271 <js::PCToLineNumber(unsigned int, unsigned char*, unsigned char*, unsigned char*, unsigned int*)+401>
=> 0x8601271 <js::PCToLineNumber(unsigned int, unsigned char*, unsigned char*, unsigned char*, unsigned int*)+401>:	movl   $0x0,0x0
   0x860127b <js::PCToLineNumber(unsigned int, unsigned char*, unsigned char*, unsigned char*, unsigned int*)+411>:	ud2    


This issue seems to be 32-bit only, could be an integer overflow of some sort. Marking s-s because it is 32-bit only and the assertion sounds potentially dangerous.

Comment 1

[Andrew McCreight [:mccr8]]

This just sounds like an overflow in computing the column number, which isn't a big deal, but I guess it could mean that there are bogus values floating around that get used in some other place, so I'm going to leave this closed, but mark it sec-audit. Please remove that rating or assign a new one if something more bad is happening.

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

Not sure who else to needinfo? here about column numbers. Setting needinfo? from Shu-yu to see how to move this forward.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> This just sounds like an overflow in computing the column number, which
> isn't a big deal, but I guess it could mean that there are bogus values
> floating around that get used in some other place, so I'm going to leave
> this closed, but mark it sec-audit. Please remove that rating or assign a
> new one if something more bad is happening.

This is indeed a *signed* 32bit overflow in computing the column number, but the actual bug is that our column handling is a piece of shit.

The bug is this: for each statement we emit in the bytecode emitter, we emit srcnotes for encoding line/col information as a delta from the previous srcnote. The previous line and column offsets are kept in variables called |currentLine| and |lastColumn|. The BCE also tries to be smart and not emit useless expressions. This fuzz test is full of useless expressions where the statement is just a number literal like |6231;| When it decides to skip emitting a useless expression, it resets |lastColumn| to 0. I don't know why it does this, it doesn't roll back the already-encoded srcnote or anything. If the next immediate statement is *also* useless, it'll encode a srcnote with the absolute column number as the column delta, since |lastColumn| is now 0. Do this 0xFFFC times, and we have a series of column deltas that easily sum up to > UINT32_MAX.

This means column numbers right now for scripts that have useless expression statements are just totally bogus. I'm too bemused right now to figure out a fix. I'll look more tomorrow.

Comment 4

[Gary Kwong [:gkw] [:nth10sd]]

(In reply to Shu-yu Guo [:shu] from comment #3)
> I'll look more tomorrow.

Assigning to Shu-yu based on this.

Is bug 857052 also likely related?

Comment 5

[Shu-yu Guo [:shu]]

This looks like a dupe of bug 857052, yes.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Don't reset column and line info when emitting useless statements in BCE.

See comment 3 for explanation. I can't figure out why we would reset line/column info when emitting useless statements right now. I removed that code and tests still pass.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8867899 [details] [diff] [review]
Don't reset column and line info when emitting useless statements in BCE.

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

r=me. But please update the comments on currentLine/lastColumn to warn that if they get out of sync with the already-emitted source notes, we can get undefined behavior... or else find some better way to stop this happening again. I don't know what that would be though.

Anyway, for sure we should assign to those fields only in the 1-2 methods that emit SRC_NEWLINE/SETLINE/COLSPAN, and nowhere else. I don't know why we emit SRC_SETLINE manually in emitCStyleFor. Seems horrible; isn't it better to call updateLineNumberNotes()?

Comment 9

[Shu-yu Guo [:shu]]

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> Comment on attachment 8867899 [details] [diff] [review]
> Don't reset column and line info when emitting useless statements in BCE.
> 
> Review of attachment 8867899 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. But please update the comments on currentLine/lastColumn to warn that
> if they get out of sync with the already-emitted source notes, we can get
> undefined behavior... or else find some better way to stop this happening
> again. I don't know what that would be though.
> 
> Anyway, for sure we should assign to those fields only in the 1-2 methods
> that emit SRC_NEWLINE/SETLINE/COLSPAN, and nowhere else. I don't know why we
> emit SRC_SETLINE manually in emitCStyleFor. Seems horrible; isn't it better
> to call updateLineNumberNotes()?

I imagine the emitCStyleFor case is due to its emitting parts out of line order, and updateLineNumberNotes only handles things in line number order with the delta computation, or something?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out from inbound for failures in its own test. BTW, pulsebot doesn't comment in sec bugs, so you'll need to mark the bug manually when pushing in the future.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d58c6daabeda497d1505f7af1fc61baa0e3e6d5

https://treeherder.mozilla.org/logviewer.html#?job_id=101592200&repo=mozilla-inbound

Comment 11

[Shu-yu Guo [:shu]]

Re-pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f5bcb4ddb655

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f5bcb4ddb655

Setting Beta and ESR52 to wontfix per IRC discussion with Shu.