1736552 - `ParserBase::setFunctionEndFromCurrentToken` and `TokenStreamSpecific<..>::advance` disagree about liveness

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Julian Seward [:jseward]]

Attachment: Valgrind complainage

This is something picked up by Valgrind/Memcheck. It looks like an
inconsistency between the one-and-only assertion in
ParserBase::setFunctionEndFromCurrentToken and what's in
TokenStreamSpecific<..>::advance. The former has this:

  MOZ_ASSERT(anyChars.currentToken().type != TokenKind::Eof);

which, aside from the actual assertion, implies that "it's OK to look at
anyChars.currentToken().type right now. But V reports an uninitialised
value error on that line, and claims that the undefined value comes from
TokenStreamSpecific<..>::advance, specifically this:

  MOZ_MAKE_MEM_UNDEFINED(&cur->type, sizeof(cur->type));

That "paints" cur->type as containing undefined data. I'm assuming that
anyChars.currentToken().type overlaps the painted area (maybe exactly) and
that's why V subsequently complains.

In effect TokenStreamSpecific<..>::advance says "the type field is now
invalid" but the assertion in ParserBase::setFunctionEndFromCurrentToken
says "well, I don't care, I want to look at it anyway". Resolving the
inconsistency requires someone who understands the logic here, so I offer no
fix.

STR:

Configure/build on x86_64-linux with
CC="gcc" CXX="g++" ../src/configure --enable-debug --disable-optimize --enable-valgrind --disable-jemalloc

Then:

valgrind --track-origins=yes
./dist/bin/js --no-threads -f
/space/MOZ/MC-TALL/js/src/jit-test/lib/prologue.js
-e 'const platform='"'"'linux'"'"''
-e 'const libdir='"'"'/space/MOZ/MC-TALL/js/src/jit-test/lib/'"'"''
-e 'const scriptdir='"'"'/space/MOZ/MC-TALL/js/src/jit-test/tests/arrow-functions/'"'"''
--module-load-path /space/MOZ/MC-TALL/js/src/jit-test/modules/
-f
/space/MOZ/MC-TALL/js/src/jit-test/tests/arrow-functions/associativity-1.js

produces the attached complaint.

Comment 1

[Tooru Fujisawa [:arai]]

Thank you!

indeed, it shouldn't use type after skipping the inner lazy function, given that operation doesn't get any token,
but just jumps to the end of the function.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Bug 1736552 - Do not read Token.type field after TokenStreamSpecific::advance. r?yulia!

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0379d2957094
Do not read Token.type field after TokenStreamSpecific::advance. r=tcampbell

Comment 4

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/0379d2957094