This is an archive of the discontinued LLVM Phabricator instance.

X86: Fix use-after-realloc in X86AsmParser::ParseIntelExpression
ClosedPublic

Authored by dexonsmith on Jan 20 2021, 7:03 PM.

Details

Summary

X86AsmParser::ParseIntelExpression has a while loop. In the body,
calls to MCAsmLexer::UnLex can force a reallocation in the MCAsmLexer's
CurToken SmallVector, invalidating saved references to
MCAsmLexer::getTok().

const MCAsmToken &Tok is such a saved reference, and this moves it
from outside the while loop to inside the body, fixing a
use-after-realloc.

Tok will still be reused across calls to Lex(), each of which
effectively destroys and constructs the pointed-to token. I'm a bit
skeptical of this usage pattern, but it seems broadly used in the
X86AsmParser (and others) so I'm leaving it alone (for now).

Somehow this bug was exposed by https://reviews.llvm.org/D94739,
resulting in test failures in dot-operator related tests in
llvm/test/tools/llvm-ml. I suspect the exposure path is related to
optimizer changes from splitting up the grow operation, but I haven't
dug all the way in. Regardless, there are already tests in tree that
cover this; they might fail consistently if we added ASan
instrumentation to SmallVector.

Diff Detail

Event Timeline

dexonsmith created this revision.Jan 20 2021, 7:03 PM
dexonsmith requested review of this revision.Jan 20 2021, 7:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 7:03 PM

(I had intended to reference https://reviews.llvm.org/D94739... not sure why it took me three tries to get the right link.)

@dblaikie, I've added you here since you reviewed the SmallVector patch that triggered the problem, but let me know if you'd rather I found another reviewer. In case it's helpful, the Lex and UnLex functions are defined at https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/MC/MCParser/MCAsmLexer.h#L77.

Remove an unrelated whitespace change.

dblaikie accepted this revision.Jan 20 2021, 9:23 PM

Sounds plausible enough to me.

Would be nice to have the asm annotations on SmallVector to back that up/make the codebase more robust.

This revision is now accepted and ready to land.Jan 20 2021, 9:23 PM