This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Done some changes to detect Uninitialized read by the char array manipulation functions
ClosedPublic

Authored by phyBrackets on Feb 24 2022, 7:42 AM.

Details

Summary

Few weeks back I was experimenting with reading the uninitialized values from src , which is actually a bug but the CSA seems to give up at that point . I was curious about that and I pinged @steakhal on the discord and according to him this seems to be a genuine issue and needs to be fix. So I goes with fixing this bug and thanks to @steakhal who help me creating this patch. This feature seems to break some tests but this was the genuine problem and the broken tests also needs to fix in certain manner. I add a test but yeah we need more tests,I'll try to add more tests.Thanks

Diff Detail

Event Timeline

phyBrackets created this revision.Feb 24 2022, 7:42 AM
phyBrackets requested review of this revision.Feb 24 2022, 7:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2022, 7:42 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
phyBrackets retitled this revision from [analyzer][NFCi] Does some changes to detect Uninitialized read by the char array manipulation functions to [analyzer][NFCi] Done some changes to detect Uninitialized read by the char array manipulation functions.Feb 24 2022, 7:59 AM

I'm looking forward to this check.

clang/test/Analysis/bstring.c
298–306

Basically, the store has 4 direct bindings for the src cluster.
At bit offset 0, 32, 64, 96, the values 1, 2, 3, 4 (ints) respectively.
However, in the memcopy modeling, we calculate the byte offset of the very last touched byte, which is byte 15.
Consequently, we will do a lookup in the store for acquiring a binding starting at bit offset 15*8, aka. 120.
However, there is no binding for that offset. What we have instead, is a binding starting at offset 96, associating an integer, which is 4 bytes long, thus this entry actually refers to the bits [96-128], so it overlaps with the byte at [120-128].
From this, we should be able to prove that the given bits must have been initialized to some value.

What I cannot remember off the top of my head, what was the type of the ER. I hope it was char, but I cannot recall.
If that was char, then we have a bug in the store.

NoQ accepted this revision.Feb 24 2022, 10:51 AM

This looks like a good check to ultimately have but we probably won't be able to move it out of alpha until the extents issue is fixed (which is going to be a fairly intrusive fix).

clang/test/Analysis/bstring.c
298–306

Yes, this is correct. It looks like we can't have this check until we address 43459.

This revision is now accepted and ready to land.Feb 24 2022, 10:51 AM
NoQ retitled this revision from [analyzer][NFCi] Done some changes to detect Uninitialized read by the char array manipulation functions to [analyzer] Done some changes to detect Uninitialized read by the char array manipulation functions.Feb 24 2022, 10:53 AM

I removed the [NFCi] marker because this commit does introduce functional change (a new checker).

NoQ added a comment.Feb 24 2022, 10:55 AM

Also I think we might be able to enable this checker for the situation when the buffer is *completely* uninitialized. It is relatively easy for RegionStore to answer whether there are any bindings in a top-level region.

steakhal requested changes to this revision.Feb 24 2022, 11:08 AM

Oh wait, should we accept this given this serious limitation?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
475–479

We also need documentation in the clang/docs/analyzer/checkers.rst.
Please also note the limitation we experienced and described here.
Also refer back to the Umbrella GitHub issue in that section.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
262
378

Remove this line.

This revision now requires changes to proceed.Feb 24 2022, 11:08 AM

@steakhal would you just help, where in the file and what exactly write to in the documentation?

NoQ added a comment.Feb 24 2022, 1:31 PM

Oh wait, should we accept this given this serious limitation?

This check lands as alpha so it's fine. We know what to do to move it out of alpha; it's somewhat difficult but definitely not impossible, so there's a way forward with this, and the code added by this patch is ultimately correct.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
601

Yeah, dumps need to be removed from the final code.

Revised the changes

steakhal added inline comments.Feb 25 2022, 12:59 AM
clang/docs/analyzer/checkers.rst
2634–2647

I was thinking about something like this.

update the documentation for the uninitialized read checker

aligned the indentation

fix some alignment issue

steakhal accepted this revision.Feb 28 2022, 2:14 AM

Approved, given that it looks and compiles fine with oxygen.
Thank you.

This revision is now accepted and ready to land.Feb 28 2022, 2:14 AM
NoQ added inline comments.Feb 28 2022, 10:18 AM
clang/test/Analysis/bstring.c
305

Hmm, given that your change suppresses some existing tests, maybe disable your checker on this file and copy the updated tests to a new file that would have the checker enabled?

phyBrackets added inline comments.Feb 28 2022, 11:27 AM
clang/test/Analysis/bstring.c
305

I don't know how will it affect that but still that's an exception and from these exceptions test cases, we are updated about how it gonna fail on some tests cases and try to work on those exceptions.

Hey @NoQ and @steakhal , can i land this patch ?

You should copy the mempcpy14, mempcpy15 test cases into a separate file, and cut the top; while disabling this alpha.unix.cstring.UninitializedRead checker in the existing test file(s) and explicitly enable in the new separated file.
This way it wouldn't interfere with the existing test cases, since it would halt the analyzer (emits a sink node).
After that, you could land it.

steakhal added inline comments.Mar 1 2022, 8:29 AM
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
377

You should make this check optional, only if the 'checker' was enabled.

Created the separate test file for UninitializedRead checker

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 6:03 AM

Hey @NoQ and @steakhal , will you just check , if everything looks fine or not! The test cases run successfully tho now!

Please mark the done comments as "done" with the corresponding button prior to submitting an update.

clang/test/Analysis/bstring_UninitRead.c
68–86

I don't think you should copy these as well.
The matching logic is already tested in bstring.c.

That being said, a single RUN line should be enough in this file.

92–95
phyBrackets added inline comments.Mar 2 2022, 8:28 AM
clang/test/Analysis/bstring_UninitRead.c
68–86

I'm not able to run test successfully with single RUN ,

// RUN: -analyzer-checker=alpha.unix.cstring.UninitializedRead
phyBrackets marked 6 inline comments as done.Mar 2 2022, 8:30 AM
steakhal added inline comments.Mar 2 2022, 9:14 AM
clang/test/Analysis/bstring_UninitRead.c
2–85

I was thinking about something like this.

phyBrackets added inline comments.Mar 2 2022, 9:17 AM
clang/test/Analysis/bstring_UninitRead.c
68–86

wait , I think I got this! Thanks

Remove unneccesary RUN and some macros definitions

phyBrackets marked 2 inline comments as done.Mar 2 2022, 9:25 AM
steakhal accepted this revision.Mar 2 2022, 9:33 AM

The core checkers should be always enabled.

clang/test/Analysis/bstring_UninitRead.c
2

enabled the core checker

phyBrackets marked an inline comment as done.Mar 2 2022, 9:41 AM
phyBrackets added inline comments.
clang/test/Analysis/bstring_UninitRead.c
2

Yeah done! Thanks

@steakhal , if everything looks fine, can i land this ?

Yup, land it!

https://github.com/llvm/llvm-project/commit/bd1917c88a32c0930864d04f4e71155dcc3fa592 , Hey @steakhal , I land it but why it is not showing the

  void emitUninitializedReadBug(CheckerContext &C, ProgramStateRef State,
                             const Expr *E) const;
``` ?
ymandel added a subscriber: ymandel.EditedMar 3 2022, 10:08 AM

Looks like this broke the build. I'm getting:

FAILED: tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/CStringChecker.cpp.o
/usr/bin/clang++-11 ... -std=c++14 -MD -MT tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/CStringChecker.cpp.o -MF tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/CStringChecker.cpp.o.d -o tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/CStringChecker.cpp.o -c .../llvm-project/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
.../llvm-project/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:376:7: error: use of undeclared identifier 'emitUninitializedReadBug'
      emitUninitializedReadBug(C, StInBound, Buffer.Expression);
      ^
/usr/local/.../llvm-project/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:592:22: error: out-of-line definition of 'emitUninitializedReadBug' does not match any declaration in '(anonymous namespace)::CStringChecker'
void CStringChecker::emitUninitializedReadBug(CheckerContext &C,
                     ^~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.
FAILED: tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/CStringChecker.cpp.o
/usr/bin/clang++-11 ... -std=c++14 -MD -MT tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/CStringChecker.cpp.o -MF tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/CStringChecker.cpp.o.d -o tools/clang/lib/StaticAnalyzer/Checkers/CMakeFiles/obj.clangStaticAnalyzerCheckers.dir/CStringChecker.cpp.o -c .../llvm-project/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
.../llvm-project/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:376:7: error: use of undeclared identifier 'emitUninitializedReadBug'
      emitUninitializedReadBug(C, StInBound, Buffer.Expression);
      ^
.../llvm-project/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:592:22: error: out-of-line definition of 'emitUninitializedReadBug' does not match any declaration in '(anonymous namespace)::CStringChecker'
void CStringChecker::emitUninitializedReadBug(CheckerContext &C,
                     ^~~~~~~~~~~~~~~~~~~~~~~~
2 errors generated.

Can you rollback until a fix is found?

dyung added a comment.Mar 3 2022, 11:52 PM

Thanks for fixing the problem with the original commit, but in the future, if you make follow-up commits, can you change the commit message to reflect what the commit contains? Having three different commits with the exact same commit message can be very confusing.

I would recommend committing changes in the morning, to give some time for the bots to chew through your commit. This way you could react to breakages and revert if needed.

Maybe it is not too late to update the clang/docs/ReleaseNotes.rst? A new checker is certainly important for the users. Many thanks!

Maybe it is not too late to update the clang/docs/ReleaseNotes.rst? A new checker is certainly important for the users. Many thanks!

Yeah done! Thanks.