629858 - strict warning "function f does not always return a value" can cause buffer overreads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: unreduced fragile testcase

I tried but somehow failed to get a Windows environment with symbols for a js shell, but nonetheless attaching what I have.

The testcase is extremely fragile but somewhat reproducible. It doesn't seem to affect debug shells. Assuming worse-case [sg:critical?] pending further analysis.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: windbg output

As per:

https://developer.mozilla.org/en/how_to_get_a_stacktrace_with_windbg#After_the_crash_or_hang

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Tested with -m and -j on TM changeset 3f23c7719100. It seems to only occur with both -m and -j but I'm not sure.

This issue (or its variants) seems pretty common on Windows when running jsfunfuzz, but getting a somewhat reliable testcase is an extreme PITA.

How one wishes to have Breakpad for js shells.

Comment 3

[David Mandelin [:dmandelin]]

Could be a dup of bug 625141. Calling it a hardblocker pending a fix from there or further analysis.

Comment 4

[David Mandelin [:dmandelin]]

Demoting because I can not repro this on either tip or 3f23c7719100.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #4)
> Demoting because I can not repro this on either tip or 3f23c7719100.

Definitely still reproduces on Windows 7 changeset 9459c4b15890. Just keep trying:

$ ./js-opt -m -j testcase.js

and one will crash once in every 5-6 runs of the command.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #5)
> $ ./js-opt -m -j testcase.js

Of course, the binary should have a .exe extension on windows...

Comment 7

[Jesse Ruderman]

Gary, do you still see this now that bug 625141 is fixed-in-tracemonkey?

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #7)
> Gary, do you still see this now that bug 625141 is fixed-in-tracemonkey?

Yes, I do still see it (once every 5-6 tries, with the testcase in comment 0) with the following changeset on tip:

http://hg.mozilla.org/tracemonkey/rev/a6d56af51d69

This includes the checkin for bug 625141.

Comment 9

[Jesse Ruderman]

Is your js shell 32-bit or 64-bit?  ./js -j -e "print(jitstats.archIs64BIT)"

Can you reproduce on any platform other than Windows?

Does using a loop (around the entire block of tryItOut calls) make the crash reliable enough to use Lithium?

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

32-bit:

$ file js-opt-32-tm-nt.exe
js-opt-32-tm-nt.exe: PE executable for MS Windows (console) Intel 80386 32-bit

$ ./js-opt-32-tm-nt.exe -j -e "print(jitstats.archIs64BIT)"
-e:1: ReferenceError: jitstats is not defined

> Can you reproduce on any platform other than Windows?

Nope, but again, I'm not entirely sure, it doesn't blow on Linux and Mac for me no matter how many times I try but the problem might still be there.

> Does using a loop (around the entire block of tryItOut calls) make the crash reliable enough to use Lithium?

Tried that, nope, it doesn't make it reliable.

Tested on TM changeset 3b3710520c0e.

Comment 11

[Jesse Ruderman]

> -e:1: ReferenceError: jitstats is not defined
Oh, right, jitstats is debug-only and this crash is opt-only.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: fragile further-reduced testcase

This may crash after 1-2 minutes, or it may not. Either way, it's further reduced. The previous earlier unreduced testcase takes 5-6 seconds to crash if it decides to.

Both tested on Windows 7 opt build on TM changeset a1a8cd4accba.

Comment 13

[Johnny Stenback (:jst)]

This is a sg:critical bug, with a testcase. That say it needs to be a hardblocker. If this is not reproducible enough, then we can argue about that, but it sounds like this reproduces for Gary, so let's at least give it a shot.

Comment 14

[David Mandelin [:dmandelin]]

I can't reproduce a crash with signature js::TypedArray::operator.

I can crash with either test case if I do many runs, but the crash is in a js.cpp error reporter function. By itself, that would definitely not be sg:critical, but the reason we crash there is that we create a report object with a bad pointer for |tokenptr|. mxr seems to say that |tokenptr| isn't used in the browser, so this wouldn't affect the browser.

Gary, can you still reproduce the original crash signature?

Comment 15

[David Mandelin [:dmandelin]]

Attachment: Patch for shell error reporter crash

This patch stops the shell crash, which could at least be helpful for fuzzing. I'm not sure of the exact cause of the error yet, but it has something to with having a 0 character in the code. The line-handling code ends up thinking the line is short, but tokenpos data refers to locations after the 0. I'm not sure if the tokenpos is for the right line, or a different line, or what. Not sure it's that important at this point, it's kind of a corner case and it's easy to step on it this way.

Comment 16

[David Mandelin [:dmandelin]]

With the patch for the error reporter issue, I'm not able to repro this in 1000 runs of the original test case or 10 runs of the longer-running one. This is on Windows 7 with a 32-bit opt shell (TM 9ecbccbaff7b, VS2008, ../configure  --enable-debug-symbols). So, unblocking for now.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #15)
> Created attachment 511566 [details] [diff] [review]
> Patch for shell error reporter crash
> 
> This patch stops the shell crash, which could at least be helpful for fuzzing.
> I'm not sure of the exact cause of the error yet, but it has something to with
> having a 0 character in the code. The line-handling code ends up thinking the
> line is short, but tokenpos data refers to locations after the 0. I'm not sure
> if the tokenpos is for the right line, or a different line, or what. Not sure
> it's that important at this point, it's kind of a corner case and it's easy to
> step on it this way.

Yes, this gets stepped on by the fuzzer on Windows very often, and the patch does immensely help to stop the crash. I'm not sure why I got a crash with another signature, but for the testcases here it seems to stop crashes, so let's get the patch in and fuzz-testing on Windows can resume :)

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I think I noticed this on ARM as well, just that it is very difficult to get a reliable testcase as per above..

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

See bug 634444, which may be related.

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to comment #19)
> See bug 634444, which may be related.

Yeah, if dmandelin's patch helps it's almost certainly bug 634444, which I've officially decided should be backed out and not included in Firefox 4.0 (see bug 635235).

Comment 21

[Nicholas Nethercote [inactive]]

Oh, I didn't realize this was triggering before bug 634444 landed.  That's not good.

It's possible that the patch for bug 588648 caused this, though it's strange that it would only start happening in late January, because that patch landed in early December.  Gary, can you either back that patch out or revert to just before it landed and see if you can reproduce the problem?  Thanks.

Comment 22

[Nicholas Nethercote [inactive]]

(In reply to comment #21)
> Gary, can you either back that patch out

(In your local repository, I mean! :)

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> It's possible that the patch for bug 588648 caused this, though it's strange
> that it would only start happening in late January, because that patch landed
> in early December.

I only started to occasionally fuzz-test on Windows from late Jan onwards..

Comment 24

[Nicholas Nethercote [inactive]]

(In reply to comment #15)
> 
> This patch stops the shell crash, which could at least be helpful for fuzzing.
> I'm not sure of the exact cause of the error yet, but it has something to with
> having a 0 character in the code. The line-handling code ends up thinking the
> line is short, but tokenpos data refers to locations after the 0.

You mean a NUL char, ie. a char with value of 0?  Where does that occur in the test case?

> I'm not sure
> if the tokenpos is for the right line, or a different line, or what. Not sure
> it's that important at this point, it's kind of a corner case and it's easy to
> step on it this way.

There's definitely some occasional weirdness in the parser and/or scanner to do with the 'index' value used in reportCompileErrorNumberVA().  Bug 635235 (see the e4x_TCO++ patch) may be the root cause, we should fix that.  If that's not the root cause, it would be good to put the above patch in as a defensive measure (with a comment explaining why the extra test is needed).

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 511566 [details] [diff] [review]
Patch for shell error reporter crash

Whee, I actually understand the patch's code now!  (I didn't when I glanced at this when it was first posted, but with bug 635235 under my belt I grok it a fair bit more.)

Bug 634444 relied on every token's information always being correct, as did previous (now again-current) code, but the getToken state space is large enough more failures-to-set and the like may lurk.  So this might be bug 635235, or it might be something else.  It's impossible to say without a rigorous proof that the E4X failure is the only one in existence, which seems a difficult thing to produce, and arguably isn't worth the time to do so.

Either way, the patch here seems to be a hackaround.  Further, given the memory-writing behavior I observed in bug 635235, I don't believe this hack would actually protect against all badness of these sorts.

One thing that might help with diagnosing failure-to-initialize-token bugs would be to not rely on overwriting default-initialized values (like the end position's lineno) for special cases.  If the end lineno hadn't been set in that case, it would have been a valgrind uninitialized-value copy that would have been easier to see.  But that means the simple cases require more cleanup drudge.  That seems a hard tradeoff to make either way.  Maybe someone else sees another idea for how to make the token-generation stuff more robust to producing faulty tokens?

Comment 26

[Nicholas Nethercote [inactive]]

(In reply to comment #25)
> 
> Bug 634444 relied on every token's information always being correct

True.  I didn't realize defensive programming was necessary here :/

> One thing that might help with diagnosing failure-to-initialize-token bugs
> would be to not rely on overwriting default-initialized values (like the end
> position's lineno) for special cases.  If the end lineno hadn't been set in
> that case, it would have been a valgrind uninitialized-value copy that would
> have been easier to see.  But that means the simple cases require more cleanup
> drudge.  That seems a hard tradeoff to make either way.  Maybe someone else
> sees another idea for how to make the token-generation stuff more robust to
> producing faulty tokens?

There's a small circular token buffer (TokenStream::tokens) and old tokens get overwritten all the time, so Valgrind wouldn't warn against uninitialized values without some annotations.  We could overwrite with some canary values instead.  But as you say, newToken() does a lot of default initialization, which defeats canaries.

One idea is to add a Token sanity checker.  It would do things like:

- If |begin| and |end| are on different lines, check it's a token that's allowed to be on multiple lines (eg. strings, xml tokens, nothing else?)
- If |begin| and |end| are on the same line, check that begin < end.

That's all I can think of right now.  There's scope for more assertions in other places too, I think.

Comment 27

[Nicholas Nethercote [inactive]]

I've worked this out.  I think it shouldn't be a hardblocker because (a) the bug is present in 1.9.1 and 1.9.2, so it's not a regression, and (b) it can only occur in the browser if javascript.options.strict is enabled.  (Note that this turns on SpiderMonkey's "strict warnings", which is similar in spirit to ES5's "strict mode" but looks for different things.)

Here's a small reproducer;  in the shell, you need to run with |-o strict| to enable strict warnings.

  function f(a) {
      if (a > 3)
          return 644;
  }

It doesn't crash for me on Linux but Valgrind complains about overrunning a buffer.  (I got the same Valgrind complaint with Gary's original test.)  It appears it can only crash the shell on Windows, which is presumably why Gary only just found it even though it's been present for ages.  As for why it manifests in js::TypedArray::operator, who knows... memory corruption?

The problem is that TokenStream::reportCompileErrorNumberVA() uses a combination of the current token (from tokens[cursor]) and the current state in TokenStream when reporting the error.  In this case, ';' is the current token, and from that we get the index (which is the column number minus one), which is 18.  But TokenStream is already on the next line by the time the strict warning is issued, and so the line length is computed as 2 (the '}' plus the newline).  We end up copying 2 chars from line 4 into the buffer used for the error, but try to index 18 chars into it.  You can see this in the actual warning:

  b.js:4: strict warning: function f does not always return a value:
  b.js:4: strict warning: }
  b.js:4: strict warning: ..................^

The carat is pointing 18 chars into line 4.

The patch I'm about to attach fixes the problem by passing in a JSParseNode, which causes its position to be used instead of the current token's position, and we end up with this warning:

  b.js:1: strict warning: function f does not always return a value

which is clearer anyway.  There are three other places where ReportBadReturn() is called, so I passed in what seemed like the right JSParseNode to each of them too.  I confirmed that Valgrind doesn't complain with Gary's original testcase once this patch is applied.

Though I think it shouldn't be a hardblocker, I'll ask for 2.0 approval once I get r+ because the patch is very small and IMO safe.

Comment 28

[Nicholas Nethercote [inactive]]

(I removed the [hardblocker] marking because it's been changed to blocking2.x since bug 634444 -- which caused the bug to be exposed more -- was backed out.)

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 62529:4e085ba15d4c)

Comment 30

[Jeff Walden [:Waldo]]

Comment on attachment 513723 [details] [diff] [review]
patch (against TM 62529:4e085ba15d4c)

>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp

> static JSBool
>-ReportBadReturn(JSContext *cx, JSTreeContext *tc, uintN flags, uintN errnum,
>+ReportBadReturn(JSContext *cx, JSTreeContext *tc, JSParseNode *pn, uintN flags, uintN errnum,
>                 uintN anonerrnum)
> {
>+    JS_ASSERT(pn);          /* we must have a pn -- see bug 629858 comment 27 */

Nix the comment, doesn't seem to add much here.  This would seem like a place for pass-by-reference -- but for the reference haters around here, alas.  ;-)

The name pn is canonical and also canonically unhelpfully informative, for a method whose behavior is not trivially obvious.  I don't have a good suggestion for a better name.


>@@ -1539,17 +1540,17 @@ Parser::functionBody()
>     } else {
>         pn = UnaryNode::create(tc);
>         if (pn) {
>             pn->pn_kid = assignExpr();
>             if (!pn->pn_kid) {
>                 pn = NULL;
>             } else {
>                 if (tc->flags & TCF_FUN_IS_GENERATOR) {
>-                    ReportBadReturn(context, tc, JSREPORT_ERROR,
>+                    ReportBadReturn(context, tc, pn, JSREPORT_ERROR,
>                                     JSMSG_BAD_GENERATOR_RETURN,
>                                     JSMSG_BAD_ANON_GENERATOR_RETURN);

For any future readers wondering how this can get hit:

js> function f() ((yield 7), 5)
;
typein:10: TypeError: generator function f returns a value:
typein:10: ;
typein:10: ..........................^

Yes, really.


Perhaps we can't test these directly (unless we wrote a shell error reporter that would save the information to the side -- maybe useful, doesn't seem important to do it now), but we could write tests that demonstrating this stuff's Valgrind-safe.  r=me if you write some testcases that trigger Valgrind warnings pre-patch and don't post-patch and dump them in a js/src/tests/*/extensions directory.

Comment 31

[Brendan Eich [:brendan]]

Jeff: const refs are not hated, mutable refs are.

OTOH, asserting pn non-null is against house style and seems silly here. The single-file scope imposed by static on ReportBadReturn makes for a small-world codebase, where one can grep for all the calls. Anyway crashing on a near null constant-offset dereference is safe.

/be

Comment 32

[Jeff Walden [:Waldo]]

Okay.

The assertion for non-null doesn't seem silly to me, because the ReportCompileError method permits a NULL pn to be passed into it.  That's what the assertion guards against: RCE trying to be clever when it's not explicitly told where to look.  The non-NULL requirement would not be clear to me, knowing that RCE accepts NULL pn, without that assertion.

Comment 33

[Brendan Eich [:brendan]]

The assertion only guards by crashing a debug build in a controlled way. But so does a guaranteed NPE. Thus the don't-assert-not-null style rule.

For a real language-level guarantee, I agree a const ref could be used. But we don't use those elsewhere in jsparse.cpp, and nul pn being valid is unusual (you're right that the report-compile-error path is where that comes up).

I suggest consulting with jimb, luke, and jorendorff, and having a plan to use const refs more consistently, rather than injecting them piecemeal, and in this bug.

/be

Comment 34

[Jeff Walden [:Waldo]]

(In reply to comment #33)
> The assertion only guards by crashing a debug build in a controlled way. But so
> does a guaranteed NPE. Thus the don't-assert-not-null style rule.

Quite true.  I have no beef with that as long as it doesn't "unduly" hinder readability.  The vast majority of the time it doesn't.  (The exception I'm thinking of is where the pointer gets passed down half a dozen levels or so before a deref happens, or perhaps where it only *might* happen.  Again, that's very rare.)

> I suggest consulting with jimb, luke, and jorendorff, and having a plan to use
> const refs more consistently, rather than injecting them piecemeal, and in this
> bug.

I'm fine with or without them as a general rule, don't need or not-need a plan one way or the other.  One just seems appropriate here, in this narrow case, for making the requirement crystal-clear in the lowest-mental-overhead way.

(Why does it feel like we've talked past each other in almost every interaction we've had in the last couple weeks?  :-( )

Comment 35

[Nicholas Nethercote [inactive]]

(In reply to comment #32)
> Okay.
> 
> The assertion for non-null doesn't seem silly to me, because the
> ReportCompileError method permits a NULL pn to be passed into it.  That's what
> the assertion guards against: RCE trying to be clever when it's not explicitly
> told where to look.  The non-NULL requirement would not be clear to me, knowing
> that RCE accepts NULL pn, without that assertion.

That's why I put it there *and* added a comment -- to strongly indicate that it wasn't just random paranoia.  But I can remove the assertion.


(In reply to comment #30)
> 
> For any future readers wondering how this can get hit:
> 
> js> function f() ((yield 7), 5)
> ;
> typein:10: TypeError: generator function f returns a value:
> typein:10: ;
> typein:10: ..........................^
> 
> Perhaps we can't test these directly (unless we wrote a shell error reporter
> that would save the information to the side -- maybe useful, doesn't seem
> important to do it now), but we could write tests that demonstrating this
> stuff's Valgrind-safe.  r=me if you write some testcases that trigger Valgrind
> warnings pre-patch and don't post-patch and dump them in a
> js/src/tests/*/extensions directory.

Thanks for the test case!  I was thinking about storing this extra context info somewhere that it's testable anyway (Shaver suggested an extra property in SyntaxError) because I've been making various changes to the scanner lately and am a bit nervous due to the lack of regression testing of error messages.

Comment 36

[Jim Blandy :jimb]

I think an assertion is the best choice here:

- In cases where a pointer will be dereferenced anyway, it's fine to let the machine's null pointer checking do the work for us --- but in this case the pointer will not be dereferenced if it is null.

- Readers should be able to infer invariants themselves by reading the code --- but ReportCompileErrorNumber permits a null pointer, misleading such efforts.

- One can't create a reference to NULL without dereferencing a NULL pointer, which is undefined, so 'const foo &' is a type-ish way of indicating that an argument must null. But nowhere do we pass JSParseNodes by reference this way.

So while I don't want us to go crazy with assertions either, the alternatives to and arguments against assertions that I'm aware of don't seem to apply here.

Contra Waldo, I would like the assertion to have a brief comment to explain why we can't just pass along RCEN's default behavior --- perhaps "This error is often detected after the token stream has moved to a different line, so without a parse node, RCEN guesses the error position poorly"?

(I probably comment too much, but I do think a reference to bugzilla should be a second choice to a brief, clear, in-place explanation.)

Comment 37

[Jim Blandy :jimb]

fourth para, please s/must null/must not be null/.

Comment 38

[Jeff Walden [:Waldo]]

Truly I don't care that much whether there is, or isn't, a comment.  Actually I made the suggestion because I thought Brendan would be happier without it, because it's not hugely more informative than /* require non-null pn */ would be, especially in concert with hg annotate.  So if it helps, pretend I never said anything about the assertion at all, and just add the couple valgrind-anticipating tests.  :-(

Comment 39

[Brendan Eich [:brendan]]

I'm sad so much ink was spilled over this. It's true that we use pointers known to be nullable or non-null, with conventions, going back many years and due to C not C++ in large part. A bit more code like that wouldn't break us old-timers, but you who are newer are within your rights to object, and the C++ switch is a big win we shouldn't overlook.

So, I agree with Jeff: use const refs to get the compiler covering your back at compile time. Do not bloat the source with comments and assertions that at best fail hard at runtime.

/be

Comment 40

[Nicholas Nethercote [inactive]]

I'm marking this as [sg:moderate] because it's very similar to bug 635235 -- in comment 27, if you make the value returned longer you can cause a read past the end of a buffer of arbitrary length, so it's a privacy risk.  Albeit a smaller one than bug 635235 because you need javascript.options.strict enabled, which is an unusual option.

Comment 41

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/17273c2c0eda

I ended up removing the assertion.  I tried using a ref-to-const but then I would have had to do a cast to get rid of the const-ness when passing 'pn' on, which was ugly, so I didn't use that.

I added a test, but it's not run under Valgrind.  The problem was that Valgrind jit-tests get their stderr output printed on the console, so you can see any Valgrind warnings.  But SpiderMonkey was also printing a warning, which is console pollution we don't want.  Non-valgrind jit-tests don't have stderr printed to the console.  In compensation, I made the return value a very long string, which maximizes the likelihood of a crash when the fix isn't present.

Comment 42

[Nicholas Nethercote [inactive]]

Attachment: landed patch

Requesting this for 1.9.1 and 1.9.2.  It should also go into 2.0.1 (or whatever it's called) -- the approval2.0? request is for 2.x.

Comment 43

[Nicholas Nethercote [inactive]]

Comment on attachment 516741 [details] [diff] [review]
landed patch

Oh, it's already marked blocking2.x;  I'll remove the approval2.0 request.

Comment 44

[Brendan Eich [:brendan]]

(In reply to comment #41)
> http://hg.mozilla.org/tracemonkey/rev/17273c2c0eda
> 
> I ended up removing the assertion.  I tried using a ref-to-const but then I
> would have had to do a cast to get rid of the const-ness when passing 'pn' on,
> which was ugly, so I didn't use that.

Would rather const-ipate harder. But yeah.

/be

Comment 45

[Daniel Veditz [:dveditz]]

Comment on attachment 516741 [details] [diff] [review]
landed patch

We don't want to land this on the branches until we know we can get it into a shipping FF4 update, holding off on the approval requests for now.

Comment 46

[Daniel Veditz [:dveditz]]

(In reply to comment #40)
> I'm marking this as [sg:moderate] because it's very similar to bug 635235
> [...] you can cause a read past the end of a buffer of arbitrary length

So the summary is better as a buffer "overread" than "overrun"?

Comment 47

[Nicholas Nethercote [inactive]]

(In reply to comment #46)
> 
> So the summary is better as a buffer "overread" than "overrun"?

Sure.

Comment 48

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

http://hg.mozilla.org/mozilla-central/rev/17273c2c0eda

Comment 49

[Daniel Veditz [:dveditz]]

Comment on attachment 516741 [details] [diff] [review]
landed patch

Moving request to next cycle

Comment 50

[Daniel Veditz [:dveditz]]

Comment on attachment 516741 [details] [diff] [review]
landed patch

Approved for 1.9.2.18, a=dveditz for release-drivers

Comment 51

[Cameron Kaiser [:spectre]]

Comment on attachment 516741 [details] [diff] [review]
landed patch

Requesting approval for 2.0 (patch applies cleanly to branch).

Comment 52

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8a583b29d673

Comment 53

[Daniel Veditz [:dveditz]]

Comment on attachment 516741 [details] [diff] [review]
landed patch

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

Comment 54

[Christian Holler (:decoder)]

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