CStringChecker, check strlcpy/strlcat
ClosedPublic

Authored by devnexen on Apr 2 2018, 12:26 PM.

Diff Detail

Repository
rC Clang
devnexen created this revision.Apr 2 2018, 12:26 PM
devnexen edited the summary of this revision. (Show Details)Apr 2 2018, 11:48 PM
george.karpenkov accepted this revision.Apr 22 2018, 6:23 AM
This revision is now accepted and ready to land.Apr 22 2018, 6:23 AM

If anybody can land for me, I would appreciate. Thanks regardless :-)

This revision was automatically updated to reflect the committed changes.
NoQ added inline comments.Apr 27 2018, 4:57 PM
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
106

The fact that the regular strcpy/strcat isn't checked for overlaps looks like an omission to me. The difference between strcpy/strcat and strlcpy/strlcat is not in how it handles overlaps.

NoQ added a comment.Apr 30 2018, 5:33 PM

Whoops - this isn't quite correct because there's one more difference between strlcpy/strlcat and the standard strcpy/strcat/strncpy/strncat: the return value. After this patch the new functions are modeled as if they return a pointer into the string, which is incorrect and in fact causes crashes.

One of the crashes is on the following code:

int foo(char *d) {
  char e[1];
  return strlcpy(e, d, sizeof(e)) >= sizeof(e);
}

...when analyzed as clang -cc1 -w -analyze -analyzer-checker=core,unix repro.c.

David, would you be willing to have a look at this problem?

Also I forgot to add the tests before committing. Sorry!

NoQ reopened this revision.May 2 2018, 1:52 PM

I reverted this for now. I'm sorry - i would have looked into this myself, but unfortunately there are other important fixes that distract me at the moment.

This revision is now accepted and ready to land.May 2 2018, 1:52 PM
NoQ requested changes to this revision.May 2 2018, 1:52 PM
This revision now requires changes to proceed.May 2 2018, 1:52 PM

Sure ! looking into it.

devnexen updated this revision to Diff 144952.May 2 2018, 4:09 PM

The returned value is the number of character copied to the dst buffer.

NoQ added a comment.May 2 2018, 4:41 PM

Yay that was fast, thanks!

Could we have tests for the return value? Like, concatenate a couple of concrete string literals and verify the return value via clang_analyzer_eval().

devnexen updated this revision to Diff 144981.May 2 2018, 9:55 PM

New test to check the length

NoQ added inline comments.May 4 2018, 5:43 PM
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1485

This crashes on the old tests for the checker. I guess that's because the normal strcpy() doesn't have three arguments (it counts from 0).

devnexen updated this revision to Diff 145352.May 4 2018, 10:29 PM
devnexen added inline comments.
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1485

True I forgot those cases.

rja accepted this revision.May 9 2018, 12:56 AM
rja added a subscriber: rja.

LG

Thanks ! I would be grateful if anybody could land it for me.

NoQ accepted this revision.May 14 2018, 3:34 PM

Thanks! I'll commit again.

This revision is now accepted and ready to land.May 14 2018, 3:34 PM
This revision was automatically updated to reflect the committed changes.
NoQ added inline comments.May 16 2018, 11:29 AM
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1560–1566

One more cornercase where the return value needs to be corrected. It'd be great to de-duplicate this code to avoid similar problems in the future.

Test case:

int foo(char *dst, const char *src) {
  return strlcpy(dst, src, 0); // no-crash
}
devnexen added inline comments.May 17 2018, 12:47 AM
lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1560–1566

Thanks for the hint ! will do a separate "PR".

alexfh added a subscriber: alexfh.May 17 2018, 4:24 AM

This patch seems to cause an assertion failure:

assert.h assertion failed at clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:427 in clang::ento::SVal clang::ento::SValBuilder::evalBinOp(clang::ento::ProgramStateRef, BinaryOperator::Opcode, clang::ento::SVal, clang::ento::SVal, clang::QualType): op == BO_Add

The stack trace is:
__assert_fail
clang::ento::SValBuilder::evalBinOp
clang::ento::SValBuilder::evalEQ
clang::ento::SValBuilder::evalEQ
::CStringChecker::assumeZero
::CStringChecker::checkNonNull
::CStringChecker::evalStrcpyCommon
::CStringChecker::evalStrcpy
::CStringChecker::evalCall
clang::ento::eval::Call::_evalCall
clang::ento::CheckerFn::operator()
clang::ento::CheckerManager::runCheckersForEvalCall
clang::ento::ExprEngine::evalCall
clang::ento::ExprEngine::VisitCallExpr
clang::ento::ExprEngine::Visit
clang::ento::ExprEngine::ProcessStmt
clang::ento::ExprEngine::processCFGElement
clang::ento::CoreEngine::HandlePostStmt
clang::ento::CoreEngine::dispatchWorkItem
clang::ento::CoreEngine::ExecuteWorkList
clang::ento::ExprEngine::ExecuteWorkList
::AnalysisConsumer::ActionExprEngine
::AnalysisConsumer::HandleCode
::AnalysisConsumer::HandleDeclsCallGraph
::AnalysisConsumer::runAnalysisOnTranslationUnit
::AnalysisConsumer::HandleTranslationUnit

I'm trying to reduce a test case.

This is reproducible in r332425.

This comment was removed by devnexen.

I was unable to reproduce both FreeBSD and Linux. Plus my changes come after checkNonNull.

I was unable to reproduce both FreeBSD and Linux. Plus my changes come after checkNonNull.

I'm not 100% sure this was caused by your patch, but the stack trace looks suspiciously similar to what was changed here. As for not being able to reproduce: do you build Clang with assertions enabled?

I admit I do not due to much longer compilation time, I ll recompile all with and will see tomorrow if I can reproduce.

devnexen added a comment.EditedMay 18 2018, 1:32 AM

I was unable to reproduce both FreeBSD and Linux. Plus my changes come after checkNonNull.

I'm not 100% sure this was caused by your patch, but the stack trace looks suspiciously similar to what was changed here. As for not being able to reproduce: do you build Clang with assertions enabled?

I was able to reproduce but also with the revision before when it has been reverted (https://github.com/llvm-mirror/clang/commit/262d2570ebab81867b46caa763a79940768d7faa#diff-b75d57d7da174f60bb0c32783975665a)