gamesh411 added inline comments.
================
Comment at: clang/docs/analyzer/checkers.rst:2361
Default propagations defined by ``GenericTaintChecker``:
-``atoi``, ``atol``, ``atoll``, ``fgetc``, ``fgetln``, ``fgets``, ``fscanf``,
``sscanf``, ``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``,
``pread``, ``read``, ``strchr``, ``strrchr``, ``tolower``, ``toupper``
+``atoi``, ``atol``, ``atoll``, ``basename``, ``dirname``, ``fgetc``,
``fgetln``, ``fgets``, ``fnmatch``, ``fread``, ``fscanf``, ``fscanf_s``,
``index``, ``inflate``, ``isalnum``, ``isalpha``, ``isascii``, ``isblank``,
``iscntrl``, ``isdigit``, ``isgraph``, ``islower``, ``isprint``, ``ispunct``,
``isspace``, ``isupper``, ``isxdigit``, ``memchr``, ``memrchr``, ``sscanf``,
``getc``, ``getc_unlocked``, ``getdelim``, ``getline``, ``getw``, ``memcmp``,
``memcpy``, ``memmem``, ``memmove``, ``mbtowc``, ``pread``, ``qsort``,
``qsort_r``, ``rawmemchr``, ``read``, ``readv``, ``recv``, ``recvfrom``,
``rindex``, ``strcasestr``, ``strchr``, ``strchrnul``, ``strcasecmp``,
``strcmp``, ``strcspn``, ``strlen``, ``strncasecmp``, ``strncmp``, ``strndup``,
``strndupa``, ``strpbrk``, ``strrchr``, ``strsep``, ``strspn``, ``strstr``,
``strtol``, ``strtoll``, ``strtoul``, ``strtoull``, ``tolower``, ``toupper``,
``ttyname``, ``ttyname_r``, ``vfscanf``, ``vfscanf``, ``wctomb``, ``wcwidth``
----------------
steakhal wrote:
> I cannot see the corresponding propagation rule.
> That being said, it would be handy to mention that this is for `zlib`
> decompression and this should be probably a taint source anyway.
>
> `vfscanf` occurs two times.
>
> `vscanf` is not mentioned here; and there are probably a couple others like
> this.
Removed `vscanf` and `vfscanf` as modeling their taint is not straightforward
and should be done in another checker.
The problem with those is that they do not use variadic arguments, but an
abstraction of those implemented by the type `va_list`, which is used to
support invocations, where the number of arguments is determined at runtime.
In addition to modeling individual v-variants we will also have to handle the
creation of `va_list` objects, and `va_start` function calls and try to reason
about which parameters will be tainted.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:561
+ {{"vscanf"}, TR::Prop({{0}}, {{}, 1})},
+ {{"vfscanf"}, TR::Prop({{0}}, {{}, 2})},
+
----------------
steakhal wrote:
> This function has exactly 3 arguments.
> I'm also puzzled how tainting `va_list` will work out. That should be modeled
> separately IMO.
> This comment applies to all of the similar `va_list` accepting functions,
> such as `vscanf`, `vfscanf`, and possibly others.
>
> That being said, I think `vscanf` is more like `scanf`, so it should be
> modeled as a taint source instead of a propagator.
removed
================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:575
+ {{"fread"}, TR::Prop({{3}}, {{0, ReturnValueIndex}})},
+ {{"readv"}, TR::Prop({{0}}, {{1, ReturnValueIndex}})},
+ {{"recv"}, TR::Prop({{0}}, {{1, ReturnValueIndex}})},
----------------
steakhal wrote:
> I'm on board with marking read operations as props/sources.
> Let's look at the declaration: `ssize_t readv(int fd, const struct iovec
> *iov, int iovcnt);`
> I'm not sure how could the pointee of `iov` be modified by this call, as its
> `const`.
> Additionally, I doubt the effectiveness of the rule, since I don't think it
> would be too likely to have a path leading to a taint sink with an `iov`
> pointer. That being said, let it be, but don't expect much from this rule :D
On second thought, this seems pointless indeed, removed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:582-583
+
+ {{"dirname"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
+ {{"basename"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
+ {{"fnmatch"}, TR::Prop({{0}}, {{ReturnValueIndex}})},
----------------
steakhal wrote:
> These should be sorted, isn't it?
swapped, thx
================
Comment at: clang/test/Analysis/taint-generic.c:399-400
+struct iovec {
+ void *iov_base; /* Starting address */
+ size_t iov_len; /* Number of bytes to transfer */
+};
----------------
steakhal wrote:
> Please use single-line comments.
> It makes debugging test files easier in some cases.
removed
================
Comment at: clang/test/Analysis/taint-generic.c:408
+ size_t read = readv(fd, iov, iovcnt);
+ // FIXME: should be able to assert that iov is also tainted
+ return 1 / read; // expected-warning {{Division by a tainted value, possibly
zero}}
----------------
steakhal wrote:
> `clang_analyzer_isTainted(*iov)`
test case removed
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120369/new/
https://reviews.llvm.org/D120369
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
