[v4,5/5] intel/tools: make sure the binary file is properly read

Submitted by andrey simiklit on Nov. 14, 2018, 4:30 p.m.

Details

Message ID 20181114163048.26309-1-asimiklit.work@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit Nov. 14, 2018, 4:30 p.m.
From: Andrii Simiklit <andrii.simiklit@globallogic.com>

1. tools/i965_disasm.c:58:4: warning:
     ignoring return value of ‘fread’,
     declared with attribute warn_unused_result
     fread(assembly, *end, 1, fp);

v2: Fixed incorrect return value check.
       ( Eric Engestrom <eric.engestrom@intel.com> )

v3: Zero size file check placed before fread with exit()
       ( Eric Engestrom <eric.engestrom@intel.com> )

v4: - Title is changed.
    - The 'size' variable was moved to top of a function scope.
    - The assertion was replaced by the proper error handling.
    - The error message on a caller side was fixed.
       ( Eric Engestrom <eric.engestrom@intel.com> )

Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
---
 src/intel/tools/i965_disasm.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/tools/i965_disasm.c b/src/intel/tools/i965_disasm.c
index 73a6760fc1..0efbdab706 100644
--- a/src/intel/tools/i965_disasm.c
+++ b/src/intel/tools/i965_disasm.c
@@ -47,17 +47,23 @@  i965_disasm_get_file_size(FILE *fp)
 static void *
 i965_disasm_read_binary(FILE *fp, size_t *end)
 {
+   size_t size;
    void *assembly;
 
    *end = i965_disasm_get_file_size(fp);
+   if (!*end)
+      return NULL;
 
    assembly = malloc(*end + 1);
    if (assembly == NULL)
       return NULL;
 
-   fread(assembly, *end, 1, fp);
+   size = fread(assembly, *end, 1, fp);
    fclose(fp);
-
+   if (!size) {
+      free(assembly);
+      return NULL;
+   }
    return assembly;
 }
 
@@ -167,7 +173,11 @@  int main(int argc, char *argv[])
 
    assembly = i965_disasm_read_binary(fp, &end);
    if (!assembly) {
-      fprintf(stderr, "Unable to allocate buffer to read binary file\n");
+      if(end)
+        fprintf(stderr, "Unable to allocate buffer to read binary file\n");
+      else
+        fprintf(stderr, "Input file is empty\n");
+
       exit(EXIT_FAILURE);
    }
 

Comments

Hello,

Could you please take a look on this v4 if it is possible?

Thanks,
Andrii.

On Wed, Nov 14, 2018 at 6:30 PM <asimiklit.work@gmail.com> wrote:

> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
>
> 1. tools/i965_disasm.c:58:4: warning:
>      ignoring return value of ‘fread’,
>      declared with attribute warn_unused_result
>      fread(assembly, *end, 1, fp);
>
> v2: Fixed incorrect return value check.
>        ( Eric Engestrom <eric.engestrom@intel.com> )
>
> v3: Zero size file check placed before fread with exit()
>        ( Eric Engestrom <eric.engestrom@intel.com> )
>
> v4: - Title is changed.
>     - The 'size' variable was moved to top of a function scope.
>     - The assertion was replaced by the proper error handling.
>     - The error message on a caller side was fixed.
>        ( Eric Engestrom <eric.engestrom@intel.com> )
>
> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> ---
>  src/intel/tools/i965_disasm.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/intel/tools/i965_disasm.c b/src/intel/tools/i965_disasm.c
> index 73a6760fc1..0efbdab706 100644
> --- a/src/intel/tools/i965_disasm.c
> +++ b/src/intel/tools/i965_disasm.c
> @@ -47,17 +47,23 @@ i965_disasm_get_file_size(FILE *fp)
>  static void *
>  i965_disasm_read_binary(FILE *fp, size_t *end)
>  {
> +   size_t size;
>     void *assembly;
>
>     *end = i965_disasm_get_file_size(fp);
> +   if (!*end)
> +      return NULL;
>
>     assembly = malloc(*end + 1);
>     if (assembly == NULL)
>        return NULL;
>
> -   fread(assembly, *end, 1, fp);
> +   size = fread(assembly, *end, 1, fp);
>     fclose(fp);
> -
> +   if (!size) {
> +      free(assembly);
> +      return NULL;
> +   }
>     return assembly;
>  }
>
> @@ -167,7 +173,11 @@ int main(int argc, char *argv[])
>
>     assembly = i965_disasm_read_binary(fp, &end);
>     if (!assembly) {
> -      fprintf(stderr, "Unable to allocate buffer to read binary file\n");
> +      if(end)
> +        fprintf(stderr, "Unable to allocate buffer to read binary
> file\n");
> +      else
> +        fprintf(stderr, "Input file is empty\n");
> +

       exit(EXIT_FAILURE);
>     }
>
> --
> 2.17.1
>
>
On 14/11/2018 16:30, asimiklit.work@gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
>
> 1. tools/i965_disasm.c:58:4: warning:
>       ignoring return value of ‘fread’,
>       declared with attribute warn_unused_result
>       fread(assembly, *end, 1, fp);
>
> v2: Fixed incorrect return value check.
>         ( Eric Engestrom <eric.engestrom@intel.com> )
>
> v3: Zero size file check placed before fread with exit()
>         ( Eric Engestrom <eric.engestrom@intel.com> )
>
> v4: - Title is changed.
>      - The 'size' variable was moved to top of a function scope.
>      - The assertion was replaced by the proper error handling.
>      - The error message on a caller side was fixed.
>         ( Eric Engestrom <eric.engestrom@intel.com> )
>
> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>


With the nit below :


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>


> ---
>   src/intel/tools/i965_disasm.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/src/intel/tools/i965_disasm.c b/src/intel/tools/i965_disasm.c
> index 73a6760fc1..0efbdab706 100644
> --- a/src/intel/tools/i965_disasm.c
> +++ b/src/intel/tools/i965_disasm.c
> @@ -47,17 +47,23 @@ i965_disasm_get_file_size(FILE *fp)
>   static void *
>   i965_disasm_read_binary(FILE *fp, size_t *end)
>   {
> +   size_t size;
>      void *assembly;
>   
>      *end = i965_disasm_get_file_size(fp);
> +   if (!*end)
> +      return NULL;
>   
>      assembly = malloc(*end + 1);
>      if (assembly == NULL)
>         return NULL;
>   
> -   fread(assembly, *end, 1, fp);
> +   size = fread(assembly, *end, 1, fp);
>      fclose(fp);
> -
> +   if (!size) {
> +      free(assembly);
> +      return NULL;
> +   }
>      return assembly;
>   }
>   
> @@ -167,7 +173,11 @@ int main(int argc, char *argv[])
>   
>      assembly = i965_disasm_read_binary(fp, &end);
>      if (!assembly) {
> -      fprintf(stderr, "Unable to allocate buffer to read binary file\n");
> +      if(end)
if (end)
> +        fprintf(stderr, "Unable to allocate buffer to read binary file\n");
> +      else
> +        fprintf(stderr, "Input file is empty\n");
> +
>         exit(EXIT_FAILURE);
>      }
>
On Friday, 2018-11-30 15:47:11 +0000, Lionel Landwerlin wrote:
> On 14/11/2018 16:30, asimiklit.work@gmail.com wrote:
> > From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> > 
> > 1. tools/i965_disasm.c:58:4: warning:
> >       ignoring return value of ‘fread’,
> >       declared with attribute warn_unused_result
> >       fread(assembly, *end, 1, fp);
> > 
> > v2: Fixed incorrect return value check.
> >         ( Eric Engestrom <eric.engestrom@intel.com> )
> > 
> > v3: Zero size file check placed before fread with exit()
> >         ( Eric Engestrom <eric.engestrom@intel.com> )
> > 
> > v4: - Title is changed.
> >      - The 'size' variable was moved to top of a function scope.
> >      - The assertion was replaced by the proper error handling.
> >      - The error message on a caller side was fixed.
> >         ( Eric Engestrom <eric.engestrom@intel.com> )
> > 
> > Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> 
> 
> With the nit below :
> 
> 
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

I'll change that as I push it in a minute :)
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>

> 
> 
> > ---
> >   src/intel/tools/i965_disasm.c | 16 +++++++++++++---
> >   1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/intel/tools/i965_disasm.c b/src/intel/tools/i965_disasm.c
> > index 73a6760fc1..0efbdab706 100644
> > --- a/src/intel/tools/i965_disasm.c
> > +++ b/src/intel/tools/i965_disasm.c
> > @@ -47,17 +47,23 @@ i965_disasm_get_file_size(FILE *fp)
> >   static void *
> >   i965_disasm_read_binary(FILE *fp, size_t *end)
> >   {
> > +   size_t size;
> >      void *assembly;
> >      *end = i965_disasm_get_file_size(fp);
> > +   if (!*end)
> > +      return NULL;
> >      assembly = malloc(*end + 1);
> >      if (assembly == NULL)
> >         return NULL;
> > -   fread(assembly, *end, 1, fp);
> > +   size = fread(assembly, *end, 1, fp);
> >      fclose(fp);
> > -
> > +   if (!size) {
> > +      free(assembly);
> > +      return NULL;
> > +   }
> >      return assembly;
> >   }
> > @@ -167,7 +173,11 @@ int main(int argc, char *argv[])
> >      assembly = i965_disasm_read_binary(fp, &end);
> >      if (!assembly) {
> > -      fprintf(stderr, "Unable to allocate buffer to read binary file\n");
> > +      if(end)
> if (end)
> > +        fprintf(stderr, "Unable to allocate buffer to read binary file\n");
> > +      else
> > +        fprintf(stderr, "Input file is empty\n");
> > +
> >         exit(EXIT_FAILURE);
> >      }
> 
>
On Fri, Nov 30, 2018 at 5:49 PM Eric Engestrom <eric.engestrom@intel.com>
wrote:

> On Friday, 2018-11-30 15:47:11 +0000, Lionel Landwerlin wrote:
> > On 14/11/2018 16:30, asimiklit.work@gmail.com wrote:
> > > From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> > >
> > > 1. tools/i965_disasm.c:58:4: warning:
> > >       ignoring return value of ‘fread’,
> > >       declared with attribute warn_unused_result
> > >       fread(assembly, *end, 1, fp);
> > >
> > > v2: Fixed incorrect return value check.
> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
> > >
> > > v3: Zero size file check placed before fread with exit()
> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
> > >
> > > v4: - Title is changed.
> > >      - The 'size' variable was moved to top of a function scope.
> > >      - The assertion was replaced by the proper error handling.
> > >      - The error message on a caller side was fixed.
> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
> > >
> > > Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> >
> >
> > With the nit below :
> >
> >
> > Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>
> I'll change that as I push it in a minute :)
> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
>

Thanks a lot for reviews :)


>
> >
> >
> > > ---
> > >   src/intel/tools/i965_disasm.c | 16 +++++++++++++---
> > >   1 file changed, 13 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/intel/tools/i965_disasm.c
> b/src/intel/tools/i965_disasm.c
> > > index 73a6760fc1..0efbdab706 100644
> > > --- a/src/intel/tools/i965_disasm.c
> > > +++ b/src/intel/tools/i965_disasm.c
> > > @@ -47,17 +47,23 @@ i965_disasm_get_file_size(FILE *fp)
> > >   static void *
> > >   i965_disasm_read_binary(FILE *fp, size_t *end)
> > >   {
> > > +   size_t size;
> > >      void *assembly;
> > >      *end = i965_disasm_get_file_size(fp);
> > > +   if (!*end)
> > > +      return NULL;
> > >      assembly = malloc(*end + 1);
> > >      if (assembly == NULL)
> > >         return NULL;
> > > -   fread(assembly, *end, 1, fp);
> > > +   size = fread(assembly, *end, 1, fp);
> > >      fclose(fp);
> > > -
> > > +   if (!size) {
> > > +      free(assembly);
> > > +      return NULL;
> > > +   }
> > >      return assembly;
> > >   }
> > > @@ -167,7 +173,11 @@ int main(int argc, char *argv[])
> > >      assembly = i965_disasm_read_binary(fp, &end);
> > >      if (!assembly) {
> > > -      fprintf(stderr, "Unable to allocate buffer to read binary
> file\n");
> > > +      if(end)
> > if (end)
> > > +        fprintf(stderr, "Unable to allocate buffer to read binary
> file\n");
> > > +      else
> > > +        fprintf(stderr, "Input file is empty\n");
> > > +
> > >         exit(EXIT_FAILURE);
> > >      }
> >
> >
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
Hello,

I'm sorry for disturbing, could I ask you to push this patch if it
possible? )))

Regards,
Andrii.

On Fri, Nov 30, 2018 at 6:19 PM andrey simiklit <asimiklit.work@gmail.com>
wrote:

> On Fri, Nov 30, 2018 at 5:49 PM Eric Engestrom <eric.engestrom@intel.com>
> wrote:
>
>> On Friday, 2018-11-30 15:47:11 +0000, Lionel Landwerlin wrote:
>> > On 14/11/2018 16:30, asimiklit.work@gmail.com wrote:
>> > > From: Andrii Simiklit <andrii.simiklit@globallogic.com>
>> > >
>> > > 1. tools/i965_disasm.c:58:4: warning:
>> > >       ignoring return value of ‘fread’,
>> > >       declared with attribute warn_unused_result
>> > >       fread(assembly, *end, 1, fp);
>> > >
>> > > v2: Fixed incorrect return value check.
>> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
>> > >
>> > > v3: Zero size file check placed before fread with exit()
>> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
>> > >
>> > > v4: - Title is changed.
>> > >      - The 'size' variable was moved to top of a function scope.
>> > >      - The assertion was replaced by the proper error handling.
>> > >      - The error message on a caller side was fixed.
>> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
>> > >
>> > > Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
>> >
>> >
>> > With the nit below :
>> >
>> >
>> > Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>
>> I'll change that as I push it in a minute :)
>> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
>>
>
> Thanks a lot for reviews :)
>
>
>>
>> >
>> >
>> > > ---
>> > >   src/intel/tools/i965_disasm.c | 16 +++++++++++++---
>> > >   1 file changed, 13 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/src/intel/tools/i965_disasm.c
>> b/src/intel/tools/i965_disasm.c
>> > > index 73a6760fc1..0efbdab706 100644
>> > > --- a/src/intel/tools/i965_disasm.c
>> > > +++ b/src/intel/tools/i965_disasm.c
>> > > @@ -47,17 +47,23 @@ i965_disasm_get_file_size(FILE *fp)
>> > >   static void *
>> > >   i965_disasm_read_binary(FILE *fp, size_t *end)
>> > >   {
>> > > +   size_t size;
>> > >      void *assembly;
>> > >      *end = i965_disasm_get_file_size(fp);
>> > > +   if (!*end)
>> > > +      return NULL;
>> > >      assembly = malloc(*end + 1);
>> > >      if (assembly == NULL)
>> > >         return NULL;
>> > > -   fread(assembly, *end, 1, fp);
>> > > +   size = fread(assembly, *end, 1, fp);
>> > >      fclose(fp);
>> > > -
>> > > +   if (!size) {
>> > > +      free(assembly);
>> > > +      return NULL;
>> > > +   }
>> > >      return assembly;
>> > >   }
>> > > @@ -167,7 +173,11 @@ int main(int argc, char *argv[])
>> > >      assembly = i965_disasm_read_binary(fp, &end);
>> > >      if (!assembly) {
>> > > -      fprintf(stderr, "Unable to allocate buffer to read binary
>> file\n");
>> > > +      if(end)
>> > if (end)
>> > > +        fprintf(stderr, "Unable to allocate buffer to read binary
>> file\n");
>> > > +      else
>> > > +        fprintf(stderr, "Input file is empty\n");
>> > > +
>> > >         exit(EXIT_FAILURE);
>> > >      }
>> >
>> >
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
On Tuesday, 2018-12-04 15:36:28 +0200, andrey simiklit wrote:
> Hello,
> 
> I'm sorry for disturbing, could I ask you to push this patch if it
> possible? )))

Sorry yes, thanks for the reminder :)
I just pushed it!

> 
> Regards,
> Andrii.
> 
> On Fri, Nov 30, 2018 at 6:19 PM andrey simiklit <asimiklit.work@gmail.com>
> wrote:
> 
> > On Fri, Nov 30, 2018 at 5:49 PM Eric Engestrom <eric.engestrom@intel.com>
> > wrote:
> >
> >> On Friday, 2018-11-30 15:47:11 +0000, Lionel Landwerlin wrote:
> >> > On 14/11/2018 16:30, asimiklit.work@gmail.com wrote:
> >> > > From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> >> > >
> >> > > 1. tools/i965_disasm.c:58:4: warning:
> >> > >       ignoring return value of ‘fread’,
> >> > >       declared with attribute warn_unused_result
> >> > >       fread(assembly, *end, 1, fp);
> >> > >
> >> > > v2: Fixed incorrect return value check.
> >> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
> >> > >
> >> > > v3: Zero size file check placed before fread with exit()
> >> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
> >> > >
> >> > > v4: - Title is changed.
> >> > >      - The 'size' variable was moved to top of a function scope.
> >> > >      - The assertion was replaced by the proper error handling.
> >> > >      - The error message on a caller side was fixed.
> >> > >         ( Eric Engestrom <eric.engestrom@intel.com> )
> >> > >
> >> > > Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
> >> >
> >> >
> >> > With the nit below :
> >> >
> >> >
> >> > Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >>
> >> I'll change that as I push it in a minute :)
> >> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
> >>
> >
> > Thanks a lot for reviews :)
> >
> >
> >>
> >> >
> >> >
> >> > > ---
> >> > >   src/intel/tools/i965_disasm.c | 16 +++++++++++++---
> >> > >   1 file changed, 13 insertions(+), 3 deletions(-)
> >> > >
> >> > > diff --git a/src/intel/tools/i965_disasm.c
> >> b/src/intel/tools/i965_disasm.c
> >> > > index 73a6760fc1..0efbdab706 100644
> >> > > --- a/src/intel/tools/i965_disasm.c
> >> > > +++ b/src/intel/tools/i965_disasm.c
> >> > > @@ -47,17 +47,23 @@ i965_disasm_get_file_size(FILE *fp)
> >> > >   static void *
> >> > >   i965_disasm_read_binary(FILE *fp, size_t *end)
> >> > >   {
> >> > > +   size_t size;
> >> > >      void *assembly;
> >> > >      *end = i965_disasm_get_file_size(fp);
> >> > > +   if (!*end)
> >> > > +      return NULL;
> >> > >      assembly = malloc(*end + 1);
> >> > >      if (assembly == NULL)
> >> > >         return NULL;
> >> > > -   fread(assembly, *end, 1, fp);
> >> > > +   size = fread(assembly, *end, 1, fp);
> >> > >      fclose(fp);
> >> > > -
> >> > > +   if (!size) {
> >> > > +      free(assembly);
> >> > > +      return NULL;
> >> > > +   }
> >> > >      return assembly;
> >> > >   }
> >> > > @@ -167,7 +173,11 @@ int main(int argc, char *argv[])
> >> > >      assembly = i965_disasm_read_binary(fp, &end);
> >> > >      if (!assembly) {
> >> > > -      fprintf(stderr, "Unable to allocate buffer to read binary
> >> file\n");
> >> > > +      if(end)
> >> > if (end)
> >> > > +        fprintf(stderr, "Unable to allocate buffer to read binary
> >> file\n");
> >> > > +      else
> >> > > +        fprintf(stderr, "Input file is empty\n");
> >> > > +
> >> > >         exit(EXIT_FAILURE);
> >> > >      }
> >> >
> >> >
> >> _______________________________________________
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >>
> >