[Qemu-devel] [PATCH] fw_cfg: check return value of fread()

[Qemu-devel] [PATCH] fw_cfg: check return value of fread()

[Pavel Borzenkov]

Spotted by Clang Analyzer

Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
---
 hw/fw_cfg.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
index 8df265c..1125e7d 100644
--- a/hw/fw_cfg.c
+++ b/hw/fw_cfg.c
@@ -113,6 +113,13 @@ static FILE *probe_splashfile(char *filename, int *file_sizep, int *file_typep)
     if (file_type == BMP_FILE) {
         fseek(fp, 28, SEEK_SET);
         fop_ret = fread(buf, 1, 2, fp);
+        if (fop_ret != 2) {
+            error_report("Could not read bpp value from '%s': %s",
+                         filename, strerror(errno));
+            fclose(fp);
+            fp = NULL;
+            return fp;
+        }
         bmp_bpp = (buf[0] + (buf[1] << 8)) & 0xffff;
         if (bmp_bpp != 24) {
             error_report("only 24bpp bmp file is supported.");
-- 
1.7.7

Re: [Qemu-devel] [PATCH] fw_cfg: check return value of fread()

[Peter Maydell]

On 18 October 2011 18:16, Pavel Borzenkov <pavel.borzenkov@gmail.com> wrote:
> Spotted by Clang Analyzer
>
> Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
> ---
>  hw/fw_cfg.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/hw/fw_cfg.c b/hw/fw_cfg.c
> index 8df265c..1125e7d 100644
> --- a/hw/fw_cfg.c
> +++ b/hw/fw_cfg.c
> @@ -113,6 +113,13 @@ static FILE *probe_splashfile(char *filename, int *file_sizep, int *file_typep)
>     if (file_type == BMP_FILE) {
>         fseek(fp, 28, SEEK_SET);
>         fop_ret = fread(buf, 1, 2, fp);
> +        if (fop_ret != 2) {
> +            error_report("Could not read bpp value from '%s': %s",
> +                         filename, strerror(errno));
> +            fclose(fp);
> +            fp = NULL;
> +            return fp;
> +        }
>         bmp_bpp = (buf[0] + (buf[1] << 8)) & 0xffff;
>         if (bmp_bpp != 24) {
>             error_report("only 24bpp bmp file is supported.");

Yuck, this code again. We should just replace it with
g_file_get_contents() and looking at the resulting buffer.
That would be 10% of the code and much less bug-ridden.

FWIW, strictly speaking there isn't a need to check the result
of the fread() because if we don't read the data then buf[] will
still have the BMP_FILE signature in it and the != 24 check will
fail.

Not checking the return code from that fseek(), on the other hand...

-- PMM