Issue 274650043: i#1734 drfuzz: add support for fuzzing data in a struct
DescriptionCommit log for first patchset:
---------------
i#1734 drfuzz: add support for fuzzing data in a struct
- add -fuzz_data_offset, -fuzz_size_offset, and -fuzz_data_size options
- update find_target_buffer to obtain fuzzing data via data_offset
- add test for -fuzz_data_offset and -fuzz_size_offset
---------------
Patch Set 1 #Patch Set 2 : update reviewer #
Total comments: 27
MessagesTotal messages: 11
Commit log for latest patchset: --------------- i#1734 drfuzz: add support for fuzzing data in a struct - add -fuzz_data_offset, -fuzz_size_offset, and -fuzz_data_size options - update find_target_buffer to obtain fuzzing data via data_offset - add test for -fuzz_data_offset and -fuzz_size_offset Review-URL: https://codereview.appspot.com/274650043 ---------------
Sign in to reply to this message.
https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c File drmemory/fuzzer.c (right): https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode128 drmemory/fuzzer.c:128: int data_offset; /* the offset of data ptr field */ The names are too close: "data_idx" and "data_offset"? It seems like ptr or pointer or indirection needs to be in the field name. I would also group each set of mutually exclusive ones under a comment. https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode856 drmemory/fuzzer.c:856: /* we handle data argument first for possible size_offset use later */ ?? Do you mean data_offset? https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode882 drmemory/fuzzer.c:882: /* check if the data size is reasonable and give an warning if not */ The comment you deleted seems to be better written (this new one has a grammar error) and contains extra info about why we should exit -- seems best to keep the original. https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode893 drmemory/fuzzer.c:893: /* We have arg_ptr pointing to a struct/object. The mutate data is pointed "The data to mutate is pointed at by a field in the struct" https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode895 drmemory/fuzzer.c:895: * Should we save/restore all the fields of the objec/struct? spelling https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode896 drmemory/fuzzer.c:896: * If yes, what's the size. Now for simplicity, we do not save/restore them. xref i# https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h File drmemory/optionsx.h (right): https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:619: "The offset of the data pointer field in the in argument specified by -fuzz_data_idx.", "in the in"? https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:619: "The offset of the data pointer field in the in argument specified by -fuzz_data_idx.", Please update fuzzer.dox as well, explaining the different options here. It also needs to be updated to reflect the separate argument method that does not require the complex coded descriptor. https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:619: "The offset of the data pointer field in the in argument specified by -fuzz_data_idx.", This does not seem clear and needs further explanation. The parameter name is also rather vague: maybe fuzz_buffer_indirection_field or sthg. This also needs to explain how it differs from -fuzz_buffer_offset and -fuzz_data_idx: are they mutually exclusive, can they be layered, etc. https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:622: "The offset of the data size field in the in argument specified by -fuzz_data_idx.", This needs clearer documentation. First, what is "the data". Second, how does this relate to -fuzz_data_size and -fuzz_buffer_fixed_size and -fuzz_size_idx: are they mutually exclusive, which takes precedence, etc. https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:624: OPTION_CLIENT_SCOPE(drmemscope, fuzz_data_size, uint, 0, 0, UINT_MAX, This seems like a duplicate of the existing -fuzz_buffer_fixed_size. https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:626: "The fuzz data size. If set, any other size options will be ignored.") bug: the code in options.c raises a usage error, it does not ignore
Sign in to reply to this message.
https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c File drmemory/fuzzer.c (right): https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode128 drmemory/fuzzer.c:128: int data_offset; /* the offset of data ptr field */ On 2015/11/30 22:06:09, bruening wrote: > The names are too close: "data_idx" and "data_offset"? It seems like ptr or > pointer or indirection needs to be in the field name. I would also group each > set of mutually exclusive ones under a comment. the data_offset depends on the data_idx, I do not like the confusion either. How about a set of options: -fuzz_struct_idx // arg index for a struct (obj?) -fuzz_struct_offs_data // data offset in the struct -fuzz_struct_offs_size // size offset in the struct or something like that. https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode856 drmemory/fuzzer.c:856: /* we handle data argument first for possible size_offset use later */ On 2015/11/30 22:06:10, bruening wrote: > ?? Do you mean data_offset? It is size_offset, because we need first get the struct arg pointer, and then we can use it to get the size via size_offset at line 869. https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode882 drmemory/fuzzer.c:882: /* check if the data size is reasonable and give an warning if not */ On 2015/11/30 22:06:10, bruening wrote: > The comment you deleted seems to be better written (this new one has a grammar > error) and contains extra info about why we should exit -- seems best to keep > the original. I thought the comment was wrong on the exit part, it gives a warning instead of crash. should we exit or just give a warning? https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode893 drmemory/fuzzer.c:893: /* We have arg_ptr pointing to a struct/object. The mutate data is pointed On 2015/11/30 22:06:10, bruening wrote: > "The data to mutate is pointed at by a field in the struct" Done. https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode895 drmemory/fuzzer.c:895: * Should we save/restore all the fields of the objec/struct? On 2015/11/30 22:06:10, bruening wrote: > spelling Done. https://codereview.appspot.com/274650043/diff/20001/drmemory/fuzzer.c#newcode896 drmemory/fuzzer.c:896: * If yes, what's the size. Now for simplicity, we do not save/restore them. On 2015/11/30 22:06:09, bruening wrote: > xref i# Done. file i#1830 https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h File drmemory/optionsx.h (right): https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:619: "The offset of the data pointer field in the in argument specified by -fuzz_data_idx.", On 2015/11/30 22:06:10, bruening wrote: > This does not seem clear and needs further explanation. The parameter name is > also rather vague: maybe fuzz_buffer_indirection_field or sthg. This also needs > to explain how it differs from -fuzz_buffer_offset and -fuzz_data_idx: are they > mutually exclusive, can they be layered, etc. data/buffer are not necessary mutually exclusive. It looks to me the data refer to the data passed in for execution, buffer refers to the part that to be mutated. They are the same most of time, but may be different for many cases. For consistency, I would prefer using data to refer the data passed in for fuzzing, and maybe -fuzz_mutate_buf_* for the actual buffer for mutation. https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:619: "The offset of the data pointer field in the in argument specified by -fuzz_data_idx.", On 2015/11/30 22:06:10, bruening wrote: > "in the in"? Done. https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:619: "The offset of the data pointer field in the in argument specified by -fuzz_data_idx.", On 2015/11/30 22:06:10, bruening wrote: > Please update fuzzer.dox as well, explaining the different options here. It > also needs to be updated to reflect the separate argument method that does not > require the complex coded descriptor. hmm, I think I would have a separate CL for updating fuzzer.dox https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:622: "The offset of the data size field in the in argument specified by -fuzz_data_idx.", On 2015/11/30 22:06:10, bruening wrote: > This needs clearer documentation. First, what is "the data". Second, how does > this relate to -fuzz_data_size and -fuzz_buffer_fixed_size and -fuzz_size_idx: > are they mutually exclusive, which takes precedence, etc. how about -fuzz_data_size for data size specified by user -fuzz_struct_idx, -fuzz_struct_offs_data, fuzz_struct_offs_size for struct (i#1830) -fuzz_mutate_buf_start, -fuzz_mutate_buf_end, -fuzz_mutate_buf_size for mutation start/end or fixed size. They are not necessary mutually exclusive, e.g., data could be specified, mutate_buf can be specified separately, or by default the same as data start/end/size. The only possible conflict is the fixed size vs passed in size, which we would honor fixed size, then passed in size, then mutate_buffer size (cannot be bigger than actual data size). https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:624: OPTION_CLIENT_SCOPE(drmemscope, fuzz_data_size, uint, 0, 0, UINT_MAX, On 2015/11/30 22:06:10, bruening wrote: > This seems like a duplicate of the existing -fuzz_buffer_fixed_size. Not really, this gives the size of the input data/buffer. -fuzz_buffer_fixed_size specify the max size to be mutated. https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:626: "The fuzz data size. If set, any other size options will be ignored.") On 2015/11/30 22:06:10, bruening wrote: > bug: the code in options.c raises a usage error, it does not ignore The ignore happens in fuzzer.c, i.e., fuzz_data_size is first checked, and the rest will be checked only if fuzz_data_size is not set.
Sign in to reply to this message.
https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h File drmemory/optionsx.h (right): https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:619: "The offset of the data pointer field in the in argument specified by -fuzz_data_idx.", On 2015/12/01 20:57:13, zhaoqin wrote: > On 2015/11/30 22:06:10, bruening wrote: > > Please update fuzzer.dox as well, explaining the different options here. It > > also needs to be updated to reflect the separate argument method that does not > > require the complex coded descriptor. > > hmm, I think I would have a separate CL for updating fuzzer.dox I would think that new options being added should update here and fuzzer.dox in the same CL to better link them.
Sign in to reply to this message.
BTW, it says "Reviewers: burening, ..."
Sign in to reply to this message.
On 2015/12/01 21:39:51, bruening wrote: > BTW, it says "Reviewers: burening, ..." typo on the first git review command.
Sign in to reply to this message.
On 2015/12/01 22:12:10, zhaoqin wrote: > On 2015/12/01 21:39:51, bruening wrote: > > BTW, it says "Reviewers: burening, ..." > > typo on the first git review command. You can edit it.
Sign in to reply to this message.
On 2015/12/01 21:39:15, bruening wrote: > https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h > File drmemory/optionsx.h (right): > > https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... > drmemory/optionsx.h:619: "The offset of the data pointer field in the in > argument specified by -fuzz_data_idx.", > On 2015/12/01 20:57:13, zhaoqin wrote: > > On 2015/11/30 22:06:10, bruening wrote: > > > Please update fuzzer.dox as well, explaining the different options here. It > > > also needs to be updated to reflect the separate argument method that does > not > > > require the complex coded descriptor. > > > > hmm, I think I would have a separate CL for updating fuzzer.dox > > I would think that new options being added should update here and fuzzer.dox in > the same CL to better link them. yes, I think it is better to have a separate CL for the options that has already added.
Sign in to reply to this message.
https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h File drmemory/optionsx.h (right): https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:622: "The offset of the data size field in the in argument specified by -fuzz_data_idx.", On 2015/12/01 20:57:14, zhaoqin wrote: > On 2015/11/30 22:06:10, bruening wrote: > > This needs clearer documentation. First, what is "the data". Second, how > does > > this relate to -fuzz_data_size and -fuzz_buffer_fixed_size and -fuzz_size_idx: > > are they mutually exclusive, which takes precedence, etc. > > how about > -fuzz_data_size for data size specified by user > -fuzz_struct_idx, -fuzz_struct_offs_data, fuzz_struct_offs_size for struct > (i#1830) > -fuzz_mutate_buf_start, -fuzz_mutate_buf_end, -fuzz_mutate_buf_size for mutation > start/end or fixed size. > > They are not necessary mutually exclusive, e.g., data could be specified, > mutate_buf can be specified separately, or by default the same as data > start/end/size. > The only possible conflict is the fixed size vs passed in size, which we would > honor fixed size, then passed in size, then mutate_buffer size (cannot be bigger > than actual data size). What do you think of the option name?
Sign in to reply to this message.
https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h File drmemory/optionsx.h (right): https://codereview.appspot.com/274650043/diff/20001/drmemory/optionsx.h#newco... drmemory/optionsx.h:622: "The offset of the data size field in the in argument specified by -fuzz_data_idx.", On 2015/12/08 17:04:26, zhaoqin wrote: > On 2015/12/01 20:57:14, zhaoqin wrote: > > On 2015/11/30 22:06:10, bruening wrote: > > > This needs clearer documentation. First, what is "the data". Second, how > > does > > > this relate to -fuzz_data_size and -fuzz_buffer_fixed_size and > -fuzz_size_idx: > > > are they mutually exclusive, which takes precedence, etc. > > > > how about > > -fuzz_data_size for data size specified by user > > -fuzz_struct_idx, -fuzz_struct_offs_data, fuzz_struct_offs_size for struct > > (i#1830) > > -fuzz_mutate_buf_start, -fuzz_mutate_buf_end, -fuzz_mutate_buf_size for > mutation > > start/end or fixed size. > > > > They are not necessary mutually exclusive, e.g., data could be specified, > > mutate_buf can be specified separately, or by default the same as data > > start/end/size. > > The only possible conflict is the fixed size vs passed in size, which we would > > honor fixed size, then passed in size, then mutate_buffer size (cannot be > bigger > > than actual data size). > > What do you think of the option name? I would suggest s/buf_(start|end)/offs_\1/ to make it clear these are offsets and not absolute pointers or sthg
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
