[clang] First batch of patches for the Juliet benchmark for taint improvements (PR #66074)

via cfe-commits cfe-commits at lists.llvm.org
Tue Sep 12 05:55:38 PDT 2023


https://github.com/DonatNagyE commented:

These are small and straightforward improvements, they mostly LGTM.

Note that in the commit message of "[[analyzer] Fix stdin declaration in C++ tests](https://github.com/llvm/llvm-project/pull/66074/commits/ededc22487a23a7deaa971526da8a932ea27b231)" the first line contains a typo ("shoulkd").

On the commit "[[analyzer] Fix taint sink rules for exec-like functions](https://github.com/llvm/llvm-project/pull/66074/commits/c54e1342fe93a51acce9148072d60d8783333194)" I think that the old behavior might be intentional: it reports situations when malicious actors may influence which executable will be ran, but does not report situations when the arguments passed to a (presumably trusted) executable contain user input.

Of course, there are situations where tainted arguments are also very problematic (e.g. calling `sudo`, `sh` or similar "execute something" programs), and I can accept this as a policy change, but I wouldn't call it a "Fix" of outright buggy behavior.

On the meta level, I'd also remark that this "four commits in one batch" review is cumbersome (at least for me -- I'm not accustomed to github gui), and I'd prefer to see separate pull requests for unrelated commits if that's not a serious problem for you.

In summary, I'd say that:
-  [[analyzer] Make socket accept() propagate taint](https://github.com/llvm/llvm-project/pull/66074/commits/b3215f7b2e5df91bb0a12cbe309929ee9d764b16) and [[analyzer] Propagate taint for wchar variants of some APIs](https://github.com/llvm/llvm-project/pull/66074/commits/63b6a37624935ae0bb52b830e15b015998559e06) definitely LGTM; feel free to push them separately.
- On [[analyzer] Fix stdin declaration in C++ tests](https://github.com/llvm/llvm-project/pull/66074/commits/ededc22487a23a7deaa971526da8a932ea27b231) I have an inline question but the change is acceptable as it's now.
- On [[analyzer] Fix taint sink rules for exec-like functions](https://github.com/llvm/llvm-project/pull/66074/commits/c54e1342fe93a51acce9148072d60d8783333194) I'd wait a bit to see the opinion of others about handling the `exec...` functions in a stricter manner.

https://github.com/llvm/llvm-project/pull/66074


More information about the cfe-commits mailing list