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

[Peter Maydell <peter.maydell@linaro.org>]

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