https://github.com/dkrupp closed https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/steakhal approved this pull request.
I hope in the future we will have a more satisfying solution.
LGTM.
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
dkrupp wrote:
> As I'm not a maintainer, I could not push to your branch. Here is a patch
> that I think has the missing pieces to satisfy my review.
>
https://github.com/dkrupp updated
https://github.com/llvm/llvm-project/pull/66086
>From 889c886c3eed31335531ec61ad2b48bef15414d8 Mon Sep 17 00:00:00 2001
From: Daniel Krupp
Date: Fri, 8 Sep 2023 16:57:49 +0200
Subject: [PATCH] [analyzer] TaintPropagation checker strlen() should not
propagate
steakhal wrote:
As I'm not a maintainer, I could not push to your branch.
Here is a patch that I think has the missing pieces to satisfy my review.
https://github.com/dkrupp updated
https://github.com/llvm/llvm-project/pull/66086
>From f8997b16c74543eb57b272c4dd4abca1a10d9ac7 Mon Sep 17 00:00:00 2001
From: Daniel Krupp
Date: Fri, 8 Sep 2023 16:57:49 +0200
Subject: [PATCH] [analyzer] TaintPropagation checker strlen() should not
propagate
steakhal wrote:
Request another round of review once you are happy with the content and
addressed the open comments.
On the grand scheme we are aligned.
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
steakhal wrote:
> Putting an upper bound on `strlen` is not just for `malloc`, it's also needed
> for ArrayBoundV2.
>
> As a very clear example, this [function `strfuzz_ends_with` from
>
DonatNagyE wrote:
Putting an upper bound on `strlen` is not just for `malloc`, it's also needed
for ArrayBoundV2.
As a very clear example, this [function `strfuzz_ends_with` from
steakhal wrote:
> So for me either solution would work:
> a) remove strlen() as a propagator and note it in the checker doc
> b) remove malloc() as a sink and note it in the checker doc
> c) don't do anything and live with the false positives
TBH I would prefer (b). I see removing the whole
DonatNagyE wrote:
> By looking at the disappeared reports you attached, I'm convinced that the
> `MsgTaintedBufferSize` diagnostics give little to no benefit in general. On
> the other side, I've seen good hits for OOBV2 in the presence of taint - even
> if that's rarely the case. On the
dkrupp wrote:
If we remove the malloc(..) as the taint sink, we would lose some true positive
findings where the size of the allocated
area is specified directly as a number by the attacker:
```
char *size=getenv("SIZE");
if (size){
pathbuf=(char*) malloc(atoi(size)+1); // warn: denial of
@@ -915,24 +915,6 @@ void testStrndupa(size_t n) {
clang_analyzer_isTainted_charp(result); // expected-warning {{YES}}
}
-size_t strlen(const char *s);
-void testStrlen() {
- char s[10];
- scanf("%9s", s);
-
- size_t result = strlen(s);
-
https://github.com/steakhal edited
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/steakhal requested changes to this pull request.
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
steakhal wrote:
I finished the review of this PR.
By looking at the disappeared reports you attached, I'm convinced that the
`MsgTaintedBufferSize` diagnostics give little to no benefit in general. On the
other side, I've seen good hits for OOBV2 in the presence of taint - even if
that's
steakhal wrote:
I actually wanted to propose another patch where the wchar variant of strlen
would propagate taint, BTW. I still plan to do it, we will see when I reach
that.
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing
steakhal wrote:
I can understand the frustration of the FPs. However, propagating taint there
is the right thing to do.
To me, the fault is on the diagnostic on the malloc. Those are the cause of the
FPs, thus that needs to be removed instead of the propagation.
I have this opinion even if the
https://github.com/dkrupp review_requested
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/dkrupp review_requested
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
llvmbot wrote:
@llvm/pr-subscribers-clang
Changes
strlen(..) call should not propagate taintedness,
because it brings in many false positive findings. It is a common pattern to
copy user provided input to another buffer. In these cases we always
get warnings about tainted data used as the
llvmbot wrote:
@llvm/pr-subscribers-clang-static-analyzer-1
Changes
strlen(..) call should not propagate taintedness,
because it brings in many false positive findings. It is a common pattern to
copy user provided input to another buffer. In these cases we always
get warnings about tainted
https://github.com/llvmbot labeled
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/llvmbot labeled
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/llvmbot labeled
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/dkrupp review_requested
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/dkrupp review_requested
https://github.com/llvm/llvm-project/pull/66086
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
https://github.com/dkrupp created
https://github.com/llvm/llvm-project/pull/66086:
strlen(..) call should not propagate taintedness,
because it brings in many false positive findings. It is a common pattern to
copy user provided input to another buffer. In these cases we always
get warnings
28 matches
Mail list logo
