Several fixes and small enhancements. #23
Conversation
There was a problem hiding this comment.
Hey, thanks for the code! Really appreciate the schema validation you added; I think that would be a big help for people writing harnesses. I left some comments/questions on the details. Also, regarding the ASAN build and some of the uninit protections you added to core, were you hitting errors caused by uninitialized reads or is this just proactive?
core/graph.hpp
Outdated
| while (sample >= 0) { | ||
| sample_idx += 1; | ||
| unsigned int sample_idx = 0; | ||
| while (sample >= 0 && sample_idx < tree.children.size()) { |
There was a problem hiding this comment.
looks like sample_idx needs to update inside the loop here
core/graph.hpp
Outdated
| sample -= tree.children[sample_idx].num_subtrees; | ||
| } | ||
| if (sample_idx >= tree.children.size()) { |
There was a problem hiding this comment.
curious if you hit cases where this went out of bounds; if so, may be a bug with the num_subtrees computation
There was a problem hiding this comment.
yes. I observed a OOB on tree.childern[sample_idx].
There was a problem hiding this comment.
on second thought. maybe this was just an attempt to fix the loop above.
|
|
||
| for (Node n : nodes) { | ||
| void *in_ref[n.in_ref_size()]; | ||
| void *out_ref[n.out_ref_size()]; | ||
| void *in_ref[n.in_ref_size() + 1]; |
There was a problem hiding this comment.
UBSan detects zero sized VLA on the stack as UB, so I just made sure the array has at least one element, even if it is not used.
| @@ -53,7 +53,7 @@ | |||
| ''' | |||
|
|
|||
| CPP_SAVE_REF = '''\ | |||
| out_ref[{out_ref_idx}] = reinterpret_cast<void *>({arg_name}); | |||
| out_ref[{out_ref_idx}] = (void*)({arg_name}); | |||
There was a problem hiding this comment.
I think there was a reason I used reinterpret_cast here over C-style casts, bit I can't remember the specifics. Can you explain the rationale for this change?
There was a problem hiding this comment.
I had troubles with harnessing a C library unless I used this. (because of casting a const pointer to a non-const pointer and vice-versa or something. I think const to non-const cast is forbidden with reinterpret_cast).
|
ok let me redo the bugfix. There might be some unnecessary changes. |
…rrors such as typos in the schema
* `global_init` is now a configurable piece of code specified in the schema. This allows to perform some setup once globally vs. at every testcase (`initializer` and `finalizer` are called on every `LLVMFuzzerTestOneInput`) * Output the code pieces from `global_init`, `initializer`, and `finalizer` in the testcase writer.
I believe this might have been caused by the issue with the subtree sampling in |
let me know if you want them split into separate PRs.