[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-14 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske closed 
https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-09 Thread via cfe-commits 
=?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= 
Message-ID:
In-Reply-To: 


https://github.com/DonatNagyE approved this pull request.

Thanks for adding the missing TC!

https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-09 Thread Balázs Kéri via cfe-commits 

balazske wrote:

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.

https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-09 Thread Balázs Kéri via cfe-commits 

balazske wrote:

I tested on vim and the problematic report disappeared, no other changes were 
detected.

https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-08 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske updated 
https://github.com/llvm/llvm-project/pull/71373

From 653aeb7f5b0d0f200b3f706bad770a9be643669c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Fri, 3 Nov 2023 09:48:18 +0100
Subject: [PATCH 1/2] [clang][analyzer] Improve StdLibraryFunctionsChecker
 'readlink' modeling.

The functions 'readlink' and 'readlinkat' do return 0 only if the
'bufsize' argument is 0.
---
 .../Checkers/StdLibraryFunctionsChecker.cpp   | 18 +
 .../Analysis/std-c-library-functions-POSIX.c  | 20 +++
 2 files changed, 34 insertions(+), 4 deletions(-)

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}}
+}

From d66440541d1fbbf50f5b750f306f79a00d3aaedc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Wed, 8 Nov 2023 16:17:15 +0100
Subject: [PATCH 2/2] add missing test

---
 clang/test/Analysis/std-c-library-functions-path-notes.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/clang/test/Analysis/std-c-library-functions-path-notes.c 
b/clang/test/Analysis/std-c-library-functions-path-notes.c
index d0957483c1391ad..4df00fe1e60646f 100644
--- a/clang/test/Analysis/std-c-library-functions-path-notes.c
+++ b/clang/test/Analysis/std-c-library-functions-path-notes.c
@@ -80,3 +80,12 @@ int

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-07 Thread via cfe-commits 

DonatNagyE wrote:

> 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.

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.

https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-07 Thread via cfe-commits 

https://github.com/DonatNagyE edited 
https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-07 Thread via cfe-commits 

https://github.com/DonatNagyE requested changes to this pull request.

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.

https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-07 Thread via cfe-commits 

DonatNagyE wrote:

Feel free to merge this @balazske, and thanks for the improvement!

https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-06 Thread Balazs Benics via cfe-commits 

https://github.com/steakhal approved this pull request.

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.

https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-06 Thread via cfe-commits 

https://github.com/DonatNagyE approved this pull request.

This is straightforward small change that clarifies some _very confusing_ (but 
technically true positive) bug reports that I encountered on some open source 
projects. 

https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-06 Thread via cfe-commits 

llvmbot wrote:



@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balázs Kéri (balazske)


Changes

The 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:

- (modified) clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp 
(+14-4) 
- (modified) clang/test/Analysis/std-c-library-functions-POSIX.c (+20) 


``diff
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}}
+}

``




https://github.com/llvm/llvm-project/pull/71373
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang][analyzer] Improve StdLibraryFunctionsChecker 'readlink' modeling. (PR #71373)

2023-11-06 Thread Balázs Kéri via cfe-commits 

https://github.com/balazske created 
https://github.com/llvm/llvm-project/pull/71373

The functions 'readlink' and 'readlinkat' do return 0 only if the 'bufsize' 
argument is 0.

From 653aeb7f5b0d0f200b3f706bad770a9be643669c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= 
Date: Fri, 3 Nov 2023 09:48:18 +0100
Subject: [PATCH] [clang][analyzer] Improve StdLibraryFunctionsChecker
 'readlink' modeling.

The functions 'readlink' and 'readlinkat' do return 0 only if the
'bufsize' argument is 0.
---
 .../Checkers/StdLibraryFunctionsChecker.cpp   | 18 +
 .../Analysis/std-c-library-functions-POSIX.c  | 20 +++
 2 files changed, 34 insertions(+), 4 deletions(-)

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}}
+}

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits