Repository navigation
Conversation
|
There should probably be a trac ticket for this PR. Please create it, or ask us to create it. Then the PR title should be updated. |
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The PR does not fully fix the address-only case. It rejects a VarDecl whose line holds only an address, but a FunctionDecl (also CXXMethodDecl / CXXConstructorDecl) with the same shape still segfaults. getSpelling() returns "" because typeIndex <= 0, then addtoken(tokenList, "") adds no token and returns tokenList.back(). That is nullptr when the list is empty, and nameToken->scope() dereferences it at lib/clangimport.cpp:1411. When there are earlier tokens, addtoken instead returns the previous token, which is wrong but does not crash. EnumConstantDecl 0x1 reaches the same path at :1058.
The size()-2 sites (GotoStmt, NamespaceDecl, createTokensForCXXRecord) also still read out of bounds when a node has exactly one ext token. The description says these were left alone on purpose, but the title promises more than the change does. I found nothing wrong in the checks that were added, and they don't affect valid dumps.
Testing
- I built
cppcheckandtestrunnerinpr/../testrunner TestClangImportreported 99 tests, 0 failed (1 todo), andTestGarbage TestCppcheckalso passed. Noclangis installed here, sotest/cli/clang-import_test.pyand real--clangruns could not be tested. - Fuzzing: I made a copy of
pr/built with-D_GLIBCXX_ASSERTIONSand added a temporary fuzz test. It feeds every node type inclangimport.cpplines with short ext-token lists (0x1,x,'int',0x1 <col:1>,0x1 prev, …), both at top level and inside a function body. - Reproducer test: I also added a
fromFiletest that callsparse()on a dump file. A valid dump printsint f ( ) ;, and`-VarDecl 0x1now givesInternalError: invalid AST node: VarDecl, as intended.
Reproduced crash (address-only FunctionDecl):
$ printf '`-FunctionDecl 0x1\n' > d2.txt
$ DUMP=d2.txt ./testrunner TestClangImport::fromFile # fromFile = parse(<contents of $DUMP>)
Segmentation fault (core dumped)
#1 clangimport::AstNode::addtoken (str="", valueType=true) at lib/clangimport.cpp:616
#2 clangimport::AstNode::createTokensFunctionDecl at lib/clangimport.cpp:1410
`-CXXConstructorDecl 0x1 and `-CXXMethodDecl 0x1 crash the same way. So does FunctionDecl 0x1 followed by a CompoundStmt child.
Still crashing on the size()-2 sites (the description says these were left alone):
`-FunctionDecl 0x1 <a.cpp:1:1, col:20> col:5 f 'int ()'
`-CompoundStmt 0x2 <col:9, col:20>
`-GotoStmt 0x3
This gives a segfault. ClassTemplateSpecializationDecl 0x1 segfaults at lib/clangimport.cpp:1519 (mExtTokens[mExtTokens.size() - 2]).
The fuzzer also hit many out-of-bounds reads on children (for example ConditionalOperator or CXXForRangeStmt with too few children). Those are outside the scope of this PR and I did not chase them further.
Comments
lib/clangimport.cpp:1410
This still crashes for an address-only function node, which is the same shape this PR rejects for VarDecl. For `-FunctionDecl 0x1 (and CXXMethodDecl / CXXConstructorDecl), getSpelling() returns "" because typeIndex <= 0. addtoken(tokenList, "") then adds no token and returns tokenList.back(), which is nullptr here, and the next line dereferences it. Reproduced:
`-FunctionDecl 0x1
parse() → SIGSEGV in addtoken (clangimport.cpp:616) called from here.
To fix it, throw the same AST InternalError when the spelling is empty or mExtTokens.size() < 2, like you did in createTokensVarDecl. EnumConstantDecl 0x1 reaches the same addtoken(tokenList, getSpelling()) path at line 1058.
Automated review of e267a95 by Claude. The pull request was built and tested locally.
|
Test results for commit e267a95 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8948/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
|
Good catch on the address-only declarations. 24d64ec makes createTokensFunctionDecl and the EnumConstantDecl handler throw the same AST InternalError when getSpelling() is empty, before any token is added, so addtoken() can no longer hand back a null back(). FunctionDecl, CXXConstructorDecl, CXXMethodDecl and EnumConstantDecl with only an address are now in invalidExtTokens. Verified with an ASan/UBSan build: before the change UBSan reports the null member call at clangimport.cpp:616, after it the full testrunner passes and --clang on the samples plus a few lib files never hits the new check. The size()-2 sites (GotoStmt, NamespaceDecl and createTokensForCXXRecord, which is also where ClassTemplateSpecializationDecl ends up) are guarded in #8939, which this PR follows up on. If you'd rather review one PR I can fold that one into this. The children[] indexing the fuzzer hit is a different pattern and I've left it out here. On the ticket: I don't have Trac access, so could one of you create it? I'll put the number in the title as soon as it exists. |
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
The new mExtTokens.size() < 2 guard in createTokensVarDecl still lets a two-token VarDecl through, and that input aborts cppcheck. For `-VarDecl 0x1 'int' (or 0x1 <col:1>), typeIndex stops at 1, so name = mExtTokens[0], which is the node address. addtoken("0x1") creates a number token, and setting a varId on it fails the assertion at token.cpp:195. This is the same "only an address" shape the PR rejects in other places. It needs a stricter check, for example typeIndex < 2 (no name token between the address and the type).
Apart from that, the empty-ext-token check in the AstNode constructor, the empty-spelling checks for FunctionDecl, CXXMethodDecl, CXXConstructorDecl and EnumConstantDecl, and the trailing-prev check are correct. Only lines whose ext text is all whitespace can reach the constructor check, and clang's dumper does not print such lines, so valid dumps should not be affected. My fuzzing found no other new problems. The remaining crashes it hit are the size()-2 sites and the children[] indexing, which comments.md already covers.
Testing
- Build and tests: I built
cppcheckandtestrunnerinpr/../testrunner TestClangImportreported 99 tests, 0 failed, 1 todo. - ASan build: I also built a copy of
pr/with-fsanitize=address,undefined -D_GLIBCXX_ASSERTIONS. - Running without clang: No clang is installed here, so I used a fake clang script with
--clang=. It prints a prepared dump (#!/bin/sh/cat "$DUMP"). - Fuzzing: I fed every node type defined in
clangimport.cpp(70 types) with ext tokens0x1,0x1 <col:1>and0x1 'int', both at top level and inside a function body. - Fuzz results: All the address-only shapes the PR targets now give the AST internal error. The remaining aborts are the
size()-2sites (CXXRecordDecl, ClassTemplateSpecializationDecl, GotoStmt) and thechildren[]indexing (IfStmt, SwitchStmt, WhileStmt, CXXForRangeStmt, DefaultStmt, ConstantExpr). Both are already in comments.md. The one new failure is VarDecl or FieldDecl with two ext tokens.
Reproducer (the PR's normal debug build, no sanitizers needed):
$ cat v.txt
TranslationUnitDecl 0x0 <<invalid sloc>> <invalid sloc>
`-VarDecl 0x1 'int'
$ DUMP=v.txt ./cppcheck --clang=/work/scratch/fakeclang -q t.c
cppcheck: lib/token.cpp:195: void Token::update_property_info(): Assertion `!mImpl->mVarId || mTokType == eVariable' failed.
`-VarDecl 0x1 <col:1> and `-FieldDecl 0x1 'int' abort the same way.
Automated review of 24d64ec by Claude. The pull request was built and tested locally.
|
|
||
| Token * clangimport::AstNode::createTokensVarDecl(TokenList &tokenList) | ||
| { | ||
| if (mExtTokens.size() < 2) |
There was a problem hiding this comment.
AI review
size() < 2 doesn't cover the two-token case. For `-VarDecl 0x1 'int' the loop below stops at typeIndex == 1, so name = mExtTokens[0] is the address 0x1. addtoken("0x1") creates a number token, and varDecl() then gives it a varId, which fails the assertion at token.cpp:195:
TranslationUnitDecl 0x0 <<invalid sloc>> <invalid sloc>
`-VarDecl 0x1 'int'
cppcheck: lib/token.cpp:195: void Token::update_property_info(): Assertion `!mImpl->mVarId || mTokType == eVariable' failed.
0x1 <col:1> and FieldDecl 0x1 'int' abort the same way. Throwing when typeIndex < 2 after the loop (that is, when there is no name token between the address and the type) would also cover the address-only case.
|
This pull request has merge conflicts that must be resolved. |
|
Test results for commit 24d64ec (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8948/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
Follow-up to #8939. Several
createTokenshandlers callmExtTokens.back()/front()or index relative tosize()without checking that the node line had any ext tokens, so a--clangdump line with nothing after the node name (e.g.`-IntegerLiteralfollowed by a space) reads out of bounds. The same happens for StringLiteral, CharacterLiteral, CXXBoolLiteralExpr, LabelStmt, EnumDecl, DeclRefExpr and others, for a VarDecl that only has an address (mExtTokens[typeIndex - 1]withtypeIndex0), and for a FunctionDecl ending inprev(dereferencesend()).The AstNode constructor now throws an AST internal error when the node has no ext tokens, and
createTokensVarDecl/createTokensFunctionDeclthrow the same error for the other two shapes. An address-only FunctionDecl / CXXMethodDecl / CXXConstructorDecl or EnumConstantDecl (`-FunctionDecl 0x1) is rejected as well:getSpelling()is empty there,addtoken("")adds nothing and returns a nullback(), so both handlers now check the spelling before adding any token. Real clang dumps always carry the node address, location, name and type, so they are unaffected (checked--clangon the samples and a few lib files).crash2now expects the internal error instead of silently producing no tokens. The size()-2 sites are guarded in #8939 and not touched here.I don't have Trac access, could someone create a ticket for this so I can reference it in the title?