steakhal wrote: > 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").
I'll check for typos. Thanks. I should really set up grammarly for vim - where I mostly write my commit messages. > 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. I'd lean towards that this was just a mistake in the past. > 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. I feel you. However, given that the commits touch the same file(s) (checker & test) it would pose challenges to apply the diffs out-of-order, because they likely would conflict in the git diff contexts. I tried to make compromises, of course, having this in mind. This is why this batch has barely any "observable" effect. Unlike the other half of the patches, where the patches are more debatable. > 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. I'll split off the `Fix taint sink rules for exec-like functions` change from this PR, and file a dedicated one. https://github.com/llvm/llvm-project/pull/66074 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
