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
Details
- Reviewers
steakhal NoQ Szelethus chandlerc - Commits
- rG9c300c18a4ea: [analyzer] Done some changes to detect Uninitialized read by the char array…
rG56eaf869be27: [analyzer] Done some changes to detect Uninitialized read by the char array…
rGbd1917c88a32: [analyzer] Done some changes to detect Uninitialized read by the char array…
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. 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. |
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. |
I removed the [NFCi] marker because this commit does introduce functional change (a new checker).
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.
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. | |
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp | ||
262 | ||
378 | Remove this line. |
@steakhal would you just help, where in the file and what exactly write to in the documentation?
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. |
clang/docs/analyzer/checkers.rst | ||
---|---|---|
2634–2647 | I was thinking about something like this. |
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? |
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. |
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.
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp | ||
---|---|---|
377 | You should make this check optional, only if the 'checker' was enabled. |
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. That being said, a single RUN line should be enough in this file. | |
92–95 |
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 |
clang/test/Analysis/bstring_UninitRead.c | ||
---|---|---|
2–85 | I was thinking about something like this. |
clang/test/Analysis/bstring_UninitRead.c | ||
---|---|---|
68–86 | wait , I think I got this! Thanks |
The core checkers should be always enabled.
clang/test/Analysis/bstring_UninitRead.c | ||
---|---|---|
2 |
clang/test/Analysis/bstring_UninitRead.c | ||
---|---|---|
2 | Yeah done! Thanks |
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; ``` ?
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.
Looks like it also broke lldb green dragon incremental as well: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/41865/consoleFull#-27141295349ba4694-19c4-4d7e-bec5-911270d8a58c
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!
I was thinking about something like this.