browneee 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)) { ---------------- tkuchta wrote: > browneee wrote: > > tkuchta wrote: > > > 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? > > > > > Yes, we are usually more concerned with the taint of the data, not the > > pointer. > > > > With strict dependencies: > > // If the input pointer is tainted, the output pointer would be tainted > > (because it is derived from the input pointer - maybe the same value). > > taint(s[0]) == dfsan_read_label(s, sizeof(s)) ====> taint(ret) == > > ret_label[0] > > > > // If the input data is tainted, the output data would be tainted (because > > it is derived from the input data). > > taint(s[0][0]) == MEM_TO_SHADOW(s[0])[0] ====> taint(ret[0]) == > > MEM_TO_SHADOW(ret)[0] > > > > Because s[0] == ret (or ret==null), (for the non-null case) the output > > shadow bytes are the same bytes as input shadow bytes and so these taint > > labels for the string data in shadow memory do not need to be explicitly > > propagated in this function. > > > > I think the only case actually changing/copying string data is writing a > > delimiter byte to NULL, which you handled. > I am working on the changes and I came across a strange behavior that I would > like to ask about. > > It turned out that if we do > > char *s = " ... "; > dfsan_set_label(label, &p_s, sizeof(&p_s)); > > Then, the s_label within the handler is 0, not "label". This is unexpected, > as we would like the pointer itself to be labeled here. > I believe this has something to do with the fact that the input string in > strsep is a double pointer. For example this works as expected for the > delimiter string, which is a single pointer. > It's either I'm doing something incorrectly or there is some issue with > propagating labels for double pointers. > Have you perhaps come across this behavior before? I'm not sure what p_s is in your example. Could you provide a more complete example? (and maybe all in one function, not half in __dfsw_strsep and half in another function) Here is an example showing taint labels at different levels of indirection: ``` #include <assert.h> #include <string.h> #include <sanitizer/dfsan_interface.h> int main(void) { char *s = " ... "; char **p_s = &s; char ***pp_s = &p_s; dfsan_label i_label = 1 << 0; dfsan_label j_label = 1 << 1; dfsan_label k_label = 1 << 2; dfsan_label m_label = 1 << 3; // string data dfsan_set_label(i_label, s, strlen(s)); // pointer s dfsan_set_label(j_label, &s, sizeof(s)); // pointer to pointer s dfsan_set_label(k_label, &p_s, sizeof(p_s)); // pointer to pointer to pointer s dfsan_set_label(m_label, &pp_s, sizeof(pp_s)); assert(pp_s[0][0] == s); // string data assert(dfsan_read_label(s, strlen(s)) == i_label); // pointer s assert(dfsan_read_label(&s, sizeof(s)) == j_label); // pointer to pointer s assert(dfsan_read_label(&p_s, sizeof(p_s)) == k_label); // pointer to pointer to pointer s assert(dfsan_read_label(&pp_s, sizeof(pp_s)) == m_label); return 0; } ``` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141389/new/ https://reviews.llvm.org/D141389 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits