I discussed about this with you @dergachev.a a long time ago on the mailing list ("static or dynamic code analysis for undefined behavior in sprintf") and finally took the time to implement something :)
Let me know what you think about this.
Differential D150430
Implement BufferOverlap check for sprint/snprintf ArnaudBienner on May 12 2023, 1:08 AM. Authored by
Details I discussed about this with you @dergachev.a a long time ago on the mailing list ("static or dynamic code analysis for undefined behavior in sprintf") and finally took the time to implement something :) Let me know what you think about this.
Diff Detail
Event Timeline
Comment Actions I like it. How about an implementation like this: void CStringChecker::evalSprintfCommon(CheckerContext &C, const CallExpr *CE, bool IsBounded) const { ProgramStateRef State = C.getState(); DestinationArgExpr Dest = {CE->getArg(0), 0}; const auto NumParams = CE->getCalleeDecl()->getAsFunction()->getNumParams(); assert(CE->getNumArgs() >= NumParams); auto AllArguments = llvm::make_range(CE->getArgs(), CE->getArgs() + CE->getNumArgs()); auto VariadicArguments = drop_begin(enumerate(AllArguments), NumParams); for (auto [ArgIdx, ArgExpr] : VariadicArguments) { // We consider only string buffers if (QualType PointeeTy = ArgExpr->getType()->getPointeeType(); PointeeTy.isNull() || !PointeeTy->isAnyCharacterType()) continue; SourceArgExpr Source = {ArgExpr, unsigned(ArgIdx)}; // Ensure the buffers do not overlap. SizeArgExpr SrcExprAsSizeDummy = {Source.Expression, Source.ArgumentIndex}; State = CheckOverlap( C, State, (IsBounded ? SizeArgExpr{CE->getArg(1), 1} : SrcExprAsSizeDummy), Dest, Source); if (!State) return; } C.addTransition(State); }
Maybe add a few test cases where the warning should not be present.
Comment Actions Here is the migrated version of the mentioned discussion: https://discourse.llvm.org/t/static-or-dynamic-code-analysis-for-undefined-behavior-in-sprintf/59624 Comment Actions Thanks for the review :) I implemented the suggested changes. Just one question: PointeeTy.isNull(): is this guaranteed to always work even though getType()->isAnyPointerType() == false? I'm assuming yes since the tests still pass, but wanted to confirm this is the best way to do this check for char* arguments. Comment Actions I was also surprised, but check the definition of that function. It will try to cast the nonnull type to a list of types, and return null if didnt match. Comment Actions Giving this a second thought, I feel like the initial check: if (!ArgExpr->getType()->isAnyPointerType() || !ArgExpr->getType()->getPointeeType()->isAnyCharacterType()) is better than the new one. To me it reads like "expr type is a pointer and it points to character type" which is more understandable IMHO. If you're worried about the expression being a bit long, I could move type to a temp variable: if (const QualType type = ArgExpr->getType(); !type->isAnyPointerType() || !type->getPointeeType()->isAnyCharacterType()) Though I'm not sure this is really more readable. What do you think? Any other suggestion/comment about this patch? Comment Actions @steakhal did you get a chance to look at my comment? I would really love to see this merged upstream if you think this could be a beneficial change :) Comment Actions Yea, it still looks okay. Let me know if you don't have commit access. In that case, also let me know what should be used for the commit author. Comment Actions Ah, I can see that pre-merge bots failed due to clang-format violations. Comment Actions Thanks @steakhal for the proposal :) but I pushed it myself: https://github.com/llvm/llvm-project/commit/ce97312d109b21acb97d3ea243e214f20bd87cfc I used to have svn access, and Chris kindly give me write access to the git repository. Comment Actions @steakhal should the release notes page be updated to add this? I think this could be beneficial for the users to be aware of that change. If yes, who is responsible for doing that? Developers? (me) or someone else is taking care of reviewing the list of changes since latest releases? Comment Actions TBH we don't really have processes there. And its a bit of a mess, regarding CSA. Usually we forgot to update the release notes, and at best we aggregate the changes just prior a release for the release notes. But we haven't done it for the last two releases for sure. I was thinking of asking you, but given this context we wouldn't gain much. We would still need to go through the diffs anyway. So, let not touch the notes now. |
Let's express that we expect at least 2 arguments at the callsites.