[clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. #71373
Conversation
…ing. The functions 'readlink' and 'readlinkat' do return 0 only if the 'bufsize' argument is 0.
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Kéri (balazske) ChangesThe functions 'readlink' and 'readlinkat' do return 0 only if the 'bufsize' argument is 0. Full diff: https://github.com/llvm/llvm-project/pull/71373.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index 13bb9cef5e490ed..54a41b8bd7843dd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -2865,9 +2865,14 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
Signature(ArgTypes{ConstCharPtrRestrictTy, CharPtrRestrictTy, SizeTy},
RetType{Ssize_tTy}),
Summary(NoEvalCall)
- .Case({ReturnValueCondition(LessThanOrEq, ArgNo(2)),
- ReturnValueCondition(WithinRange, Range(0, Ssize_tMax))},
+ .Case({ArgumentCondition(2, WithinRange, Range(1, IntMax)),
+ ReturnValueCondition(LessThanOrEq, ArgNo(2)),
+ ReturnValueCondition(WithinRange, Range(1, Ssize_tMax))},
ErrnoMustNotBeChecked, GenericSuccessMsg)
+ .Case({ArgumentCondition(2, WithinRange, SingleValue(0)),
+ ReturnValueCondition(WithinRange, SingleValue(0))},
+ ErrnoMustNotBeChecked,
+ "Assuming that argument 'bufsize' is 0")
.Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg)
.ArgConstraint(NotNull(ArgNo(0)))
.ArgConstraint(NotNull(ArgNo(1)))
@@ -2884,9 +2889,14 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
ArgTypes{IntTy, ConstCharPtrRestrictTy, CharPtrRestrictTy, SizeTy},
RetType{Ssize_tTy}),
Summary(NoEvalCall)
- .Case({ReturnValueCondition(LessThanOrEq, ArgNo(3)),
- ReturnValueCondition(WithinRange, Range(0, Ssize_tMax))},
+ .Case({ArgumentCondition(3, WithinRange, Range(1, IntMax)),
+ ReturnValueCondition(LessThanOrEq, ArgNo(3)),
+ ReturnValueCondition(WithinRange, Range(1, Ssize_tMax))},
ErrnoMustNotBeChecked, GenericSuccessMsg)
+ .Case({ArgumentCondition(3, WithinRange, SingleValue(0)),
+ ReturnValueCondition(WithinRange, SingleValue(0))},
+ ErrnoMustNotBeChecked,
+ "Assuming that argument 'bufsize' is 0")
.Case(ReturnsMinusOne, ErrnoNEZeroIrrelevant, GenericFailureMsg)
.ArgConstraint(ValidFileDescriptorOrAtFdcwd(ArgNo(0)))
.ArgConstraint(NotNull(ArgNo(1)))
diff --git a/clang/test/Analysis/std-c-library-functions-POSIX.c b/clang/test/Analysis/std-c-library-functions-POSIX.c
index 84ce0f21e569fb5..daa4d904c3ac5ed 100644
--- a/clang/test/Analysis/std-c-library-functions-POSIX.c
+++ b/clang/test/Analysis/std-c-library-functions-POSIX.c
@@ -205,3 +205,23 @@ void test_sendmsg(int sockfd, const struct msghdr *msg, int flags) {
ssize_t Ret = sendmsg(sockfd, msg, flags);
clang_analyzer_eval(Ret != 0); // expected-warning{{TRUE}}
}
+
+void test_readlink_bufsize_zero(char *Buf, size_t Bufsize) {
+ ssize_t Ret = readlink("path", Buf, Bufsize);
+ if (Ret == 0)
+ clang_analyzer_eval(Bufsize == 0); // expected-warning{{TRUE}}
+ else if (Ret > 0)
+ clang_analyzer_eval(Bufsize == 0); // expected-warning{{FALSE}}
+ else
+ clang_analyzer_eval(Bufsize == 0); // expected-warning{{UNKNOWN}}
+}
+
+void test_readlinkat_bufsize_zero(int fd, char *Buf, size_t Bufsize) {
+ ssize_t Ret = readlinkat(fd, "path", Buf, Bufsize);
+ if (Ret == 0)
+ clang_analyzer_eval(Bufsize == 0); // expected-warning{{TRUE}}
+ else if (Ret > 0)
+ clang_analyzer_eval(Bufsize == 0); // expected-warning{{FALSE}}
+ else
+ clang_analyzer_eval(Bufsize == 0); // expected-warning{{UNKNOWN}}
+}
|
There was a problem hiding this comment.
LGTM, but please wait for @donatnagye to have a look.
BTW I noticed that the note messages are not tested, but I'm okay to not test that I think.
|
Feel free to merge this @balazske, and thanks for the improvement! |
There was a problem hiding this comment.
Sorry for undoing my approval, but as I was reviewing your other commit #71392 I noticed that it includes a testcase (the change in clang/test/Analysis/std-c-library-functions-path-notes.c) that logically belongs to this commit.
Moreover, I think that TC shows that it would be useful to see the behavior of this change on open source projects as I fear that this change could lead to "technically TP, but not helpful for the user" reports.
Please move that TC into this commit and provide some open source results that show the behavior of this checker after this change.
Of course, I still think that this is a good and useful change, but we need to be careful because it seems that even a small change like this can produce surprising side effects.
On a second thought, AFAIK there are already other functions that have similar "argument is zero" corner cases that turn into separate execution paths, so it's unlikely that the impact from this (not too common) function would be especially severe. I'd slightly prefer to see open source results, but if you feel that they're not needed, then don't bother with collecting them. |
|
I tested on vim and the problematic report disappeared, no other changes were detected. |
|
The checker was already tested on some projects, but much more is needed to find such corner cases. It can be better to manually check the functions for cases when a 0 return value is not possible or only at a special (known) case. |
…ing. (llvm#71373) The functions 'readlink' and 'readlinkat' do return 0 only if the 'bufsize' argument is 0.
The functions 'readlink' and 'readlinkat' do return 0 only if the 'bufsize' argument is 0.