This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Fix taint propagation by remembering to the location context
ClosedPublic

Authored by steakhal on Feb 7 2022, 4:18 AM.

Details

Summary

Fixes the issue D118987 by mapping the propagation to the callsite's
LocationContext.
This way we can keep track of the in-flight propagations.

Note that empty propagation sets won't be inserted.

Diff Detail

Event Timeline

steakhal created this revision.Feb 7 2022, 4:18 AM

Sounds about right! Just a nit, otherwise LGTM.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
441

Are there any comfortable ways of making this non-global?
https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors

Also, I wonder how efficient immutable data structures are against something like std::vector, when the element type is small and the number of elements are expected to be very low. Just a thought, I don't have particularly strong views on this.

steakhal marked an inline comment as done.Feb 7 2022, 6:21 AM
steakhal added inline comments.
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
441

Thanks for catching it.
I grepped for a similar code and it seems like there is the REGISTER_SET_FACTORY_WITH_PROGRAMSTATE for exactly this.
Now I know about this :)

steakhal updated this revision to Diff 406426.Feb 7 2022, 6:21 AM
steakhal marked an inline comment as done.

using REGISTER_SET_FACTORY_WITH_PROGRAMSTATE

steakhal updated this revision to Diff 406429.Feb 7 2022, 6:26 AM

upload the right diff

NoQ added inline comments.Feb 7 2022, 9:55 AM
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
441

ImmutableList is another good candidate. I prefer these data structures to be immutable because that's a much stronger guarantee that they won't actually ever be updated while stored in the state.

clang/test/Analysis/taint-checker-callback-order-has-definition.c
3

This test will be annoying to update when more debug prints are added. It might be better to test this through debug.TaintTest or a new ExprInspection function.

NoQ accepted this revision.Feb 7 2022, 9:55 AM

Looks correct, thanks for fixing this!

This revision is now accepted and ready to land.Feb 7 2022, 9:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2022, 7:57 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Can we reopen this if the code is not upstream at this time?

I'll re-land the whole stack tomorrow, by adding the asserts requirement for the tests to make the tests pass on all bots.