[PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

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(-)

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;
-- 
2.14.2


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[George Dunlap]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10] fuzz/x86_emulate: Fix afl-harness batch mode file pointer leak

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel