[PATCH] D141389: [DFSAN] Add support for strnlen, strncat, strsep, sscanf and _tolower

[Tomasz Kuchta via Phabricator via cfe-commits]

tkuchta added inline comments.


================
Comment at: compiler-rt/lib/dfsan/dfsan_custom.cpp:213
+  char *res = strsep(s, delim);
+  s_label = dfsan_read_label(base, strlen(base));
+  if (res && (res != base)) {
----------------
browneee wrote:
> The `s_label` represents the taint label for `s` (the pointer).
> 
> This line would clobber the taint label of the pointer (`s`) with a taint label from `s[0][0..n]`.
> 
> I think this line should be deleted.
Agree, s_label represents the taint associated with the **s pointer. However I am now wondering if that is the taint wich we would like to return.
For example, if we have
if (flags().strict_data_dependencies) {
    *ret_label = res ? s_label : 0;

We would taint the return value with the value of the pointer, not the data. It means that if we operate on a string for which the characters are tainted, but the pointer itself isn't, we are likely going to return label 0. My understanding was that we are more concerned with the taint of the data, not the pointer, am I missing something?



================
Comment at: compiler-rt/lib/dfsan/dfsan_custom.cpp:214
+  s_label = dfsan_read_label(base, strlen(base));
+  if (res && (res != base)) {
+    char *token_start = res;
----------------
browneee wrote:
> I think `res && (res != base)` is never true.
> 
> Checking an implementation of strsep (http://git.musl-libc.org/cgit/musl/tree/src/string/strsep.c)
> `res` can either be `NULL` or the same value as `base`.
> 
> I think this line should be `(res != *s)`. This is different because `*s` may be changed by the call to `strsep` just above.
good point, my bad


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141389/new/

https://reviews.llvm.org/D141389