| Message ID | 20171013090016.9042-1-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 13.10.17 at 11:00, <george.dunlap@citrix.com> wrote: > Changeset XXXX introduced "batch mode" to afl-harness, which allowed With (part of) the commit hash and the title inserted here and ... > --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c > +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c > @@ -99,13 +99,17 @@ int main(int argc, char **argv) > exit(-1); > } > > - if ( !feof(fp) ) > + /* Only run the test if the input file was smaller than INPUT_SIZE */ > + if ( feof(fp) ) > + { > + LLVMFuzzerTestOneInput(input, size); > + } ... ideally with the unnecessary braces dropped here Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 10/13/2017 10:06 AM, Jan Beulich wrote: >>>> On 13.10.17 at 11:00, <george.dunlap@citrix.com> wrote: >> Changeset XXXX introduced "batch mode" to afl-harness, which allowed > > With (part of) the commit hash and the title inserted here and ... Gah. :-) > >> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c >> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c >> @@ -99,13 +99,17 @@ int main(int argc, char **argv) >> exit(-1); >> } >> >> - if ( !feof(fp) ) >> + /* Only run the test if the input file was smaller than INPUT_SIZE */ >> + if ( feof(fp) ) >> + { >> + LLVMFuzzerTestOneInput(input, size); >> + } > > ... ideally with the unnecessary braces dropped here > Reviewed-by: Jan Beulich <jbeulich@suse.com> Do you really want this to look like this? if ( ... ) foo(); else { ... } -George
On 10/13/2017 10:06 AM, Jan Beulich wrote: >>>> On 13.10.17 at 11:00, <george.dunlap@citrix.com> wrote: >> Changeset XXXX introduced "batch mode" to afl-harness, which allowed > > With (part of) the commit hash and the title inserted here and ... This should be `2b1cde7783` BTW. -George
>>> On 13.10.17 at 11:10, <george.dunlap@citrix.com> wrote: > On 10/13/2017 10:06 AM, Jan Beulich wrote: >>>>> On 13.10.17 at 11:00, <george.dunlap@citrix.com> wrote: >>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c >>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c >>> @@ -99,13 +99,17 @@ int main(int argc, char **argv) >>> exit(-1); >>> } >>> >>> - if ( !feof(fp) ) >>> + /* Only run the test if the input file was smaller than INPUT_SIZE */ >>> + if ( feof(fp) ) >>> + { >>> + LLVMFuzzerTestOneInput(input, size); >>> + } >> >> ... ideally with the unnecessary braces dropped here >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Do you really want this to look like this? > > if ( ... ) > foo(); > else > { > ... > } Yes. It's Linux and qemu who dislike non-matched if/else bodies, but our ./CODING_STYLE only says "Braces should be omitted for blocks with a single statement. e.g., if ( condition ) single_statement();" and personally I'm happy that it doesn't say anything more. Jan
On 10/13/2017 10:20 AM, Jan Beulich wrote: >>>> On 13.10.17 at 11:10, <george.dunlap@citrix.com> wrote: >> On 10/13/2017 10:06 AM, Jan Beulich wrote: >>>>>> On 13.10.17 at 11:00, <george.dunlap@citrix.com> wrote: >>>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c >>>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c >>>> @@ -99,13 +99,17 @@ int main(int argc, char **argv) >>>> exit(-1); >>>> } >>>> >>>> - if ( !feof(fp) ) >>>> + /* Only run the test if the input file was smaller than INPUT_SIZE */ >>>> + if ( feof(fp) ) >>>> + { >>>> + LLVMFuzzerTestOneInput(input, size); >>>> + } >>> >>> ... ideally with the unnecessary braces dropped here >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >> Do you really want this to look like this? >> >> if ( ... ) >> foo(); >> else >> { >> ... >> } > > Yes. It's Linux and qemu who dislike non-matched if/else bodies, > but our ./CODING_STYLE only says > > "Braces should be omitted for blocks with a single statement. e.g., > > if ( condition ) > single_statement();" > > and personally I'm happy that it doesn't say anything more. Hmm, I personally think it's ugly enough that I'd rather restructure the code to avoid it looking like that. :-) I'll see what I can do. -George
>>> On 13.10.17 at 12:23, <george.dunlap@citrix.com> wrote: > On 10/13/2017 10:20 AM, Jan Beulich wrote: >>>>> On 13.10.17 at 11:10, <george.dunlap@citrix.com> wrote: >>> On 10/13/2017 10:06 AM, Jan Beulich wrote: >>>>>>> On 13.10.17 at 11:00, <george.dunlap@citrix.com> wrote: >>>>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c >>>>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c >>>>> @@ -99,13 +99,17 @@ int main(int argc, char **argv) >>>>> exit(-1); >>>>> } >>>>> >>>>> - if ( !feof(fp) ) >>>>> + /* Only run the test if the input file was smaller than INPUT_SIZE */ >>>>> + if ( feof(fp) ) >>>>> + { >>>>> + LLVMFuzzerTestOneInput(input, size); >>>>> + } >>>> >>>> ... ideally with the unnecessary braces dropped here >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> >>> Do you really want this to look like this? >>> >>> if ( ... ) >>> foo(); >>> else >>> { >>> ... >>> } >> >> Yes. It's Linux and qemu who dislike non-matched if/else bodies, >> but our ./CODING_STYLE only says >> >> "Braces should be omitted for blocks with a single statement. e.g., >> >> if ( condition ) >> single_statement();" >> >> and personally I'm happy that it doesn't say anything more. > > Hmm, I personally think it's ugly enough that I'd rather restructure the > code to avoid it looking like that. :-) > > I'll see what I can do. Well, assuming you would think that way I've intentionally said "ideally", i.e. if you really don't want to change it, I can live with the braces. Jan
On 10/13/2017 11:31 AM, Jan Beulich wrote: >>>> On 13.10.17 at 12:23, <george.dunlap@citrix.com> wrote: >> On 10/13/2017 10:20 AM, Jan Beulich wrote: >>>>>> On 13.10.17 at 11:10, <george.dunlap@citrix.com> wrote: >>>> On 10/13/2017 10:06 AM, Jan Beulich wrote: >>>>>>>> On 13.10.17 at 11:00, <george.dunlap@citrix.com> wrote: >>>>>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c >>>>>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c >>>>>> @@ -99,13 +99,17 @@ int main(int argc, char **argv) >>>>>> exit(-1); >>>>>> } >>>>>> >>>>>> - if ( !feof(fp) ) >>>>>> + /* Only run the test if the input file was smaller than INPUT_SIZE */ >>>>>> + if ( feof(fp) ) >>>>>> + { >>>>>> + LLVMFuzzerTestOneInput(input, size); >>>>>> + } >>>>> >>>>> ... ideally with the unnecessary braces dropped here >>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> >>>> Do you really want this to look like this? >>>> >>>> if ( ... ) >>>> foo(); >>>> else >>>> { >>>> ... >>>> } >>> >>> Yes. It's Linux and qemu who dislike non-matched if/else bodies, >>> but our ./CODING_STYLE only says >>> >>> "Braces should be omitted for blocks with a single statement. e.g., >>> >>> if ( condition ) >>> single_statement();" >>> >>> and personally I'm happy that it doesn't say anything more. >> >> Hmm, I personally think it's ugly enough that I'd rather restructure the >> code to avoid it looking like that. :-) >> >> I'll see what I can do. > > Well, assuming you would think that way I've intentionally said > "ideally", i.e. if you really don't want to change it, I can live with > the braces. OK, thanks. :-) -George
Hi George, On 13/10/17 10:00, George Dunlap wrote: > Changeset XXXX introduced "batch mode" to afl-harness, which allowed > the handling of several inputs in sequence. > > Unfortunately, it introduced a file pointer leak when the file was > larger than the maximum size. Restructure the code to always close fp > if we opened it. > > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > Release exception justification: > - This is a bug fix. The problem is relatively minor, but the fix is relatively minor too. I agree here: Release-acked-by: Julien Grall <julien.grall@linaro.org> Cheers,
diff --git a/tools/fuzz/x86_instruction_emulator/afl-harness.c b/tools/fuzz/x86_instruction_emulator/afl-harness.c index d514468dd2..a2bae46d98 100644 --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c @@ -99,13 +99,17 @@ int main(int argc, char **argv) exit(-1); } - if ( !feof(fp) ) + /* Only run the test if the input file was smaller than INPUT_SIZE */ + if ( feof(fp) ) + { + LLVMFuzzerTestOneInput(input, size); + } + else { printf("Input too large\n"); /* Don't exit if we're doing batch processing */ if ( max == 1 ) exit(-1); - continue; } if ( fp != stdin ) @@ -113,8 +117,6 @@ int main(int argc, char **argv) fclose(fp); fp = NULL; } - - LLVMFuzzerTestOneInput(input, size); } return 0;
Changeset XXXX introduced "batch mode" to afl-harness, which allowed the handling of several inputs in sequence. Unfortunately, it introduced a file pointer leak when the file was larger than the maximum size. Restructure the code to always close fp if we opened it. Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- Release exception justification: - This is a bug fix. The problem is relatively minor, but the fix is relatively minor too. CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Ian Jackson <ian.jackson@citrix.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Julien Grall <julien.grall@arm.com> --- tools/fuzz/x86_instruction_emulator/afl-harness.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)