Re: [PATCH] hw/display/bcm2835_fb.c: Initialize all fields of struct

[Philippe Mathieu-Daudé]

On 6/28/20 9:54 PM, Peter Maydell wrote:
> In bcm2835_fb_mbox_push(), Coverity complains (CID 1429989) that we
> pass a pointer to a local struct to another function without
> initializing all its fields.  This is a real bug:
> bcm2835_fb_reconfigure() copies the whole of our new BCM2385FBConfig
> struct into s->config, so any fields we don't initialize will corrupt
> the state of the device.
> 
> Copy the two fields which we don't want to update (pixo and alpha)
> from the existing config so we don't accidentally change them.
> 
> Fixes: cfb7ba983857e40e88
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
> Not sure why this wasn't a visible bug -- alpha isn't used,
> but if pixo changes from zero to non-zero we flip from
> RGB to BGR...
> ---
>  hw/display/bcm2835_fb.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/display/bcm2835_fb.c b/hw/display/bcm2835_fb.c
> index c6263808a27..7c0e5eef2d5 100644
> --- a/hw/display/bcm2835_fb.c
> +++ b/hw/display/bcm2835_fb.c
> @@ -282,6 +282,10 @@ static void bcm2835_fb_mbox_push(BCM2835FBState *s, 
> uint32_t value)
>      newconf.base = s->vcram_base | (value & 0xc0000000);
>      newconf.base += BCM2835_FB_OFFSET;
>  
> +    /* Copy fields which we don't want to change from the existing config */
> +    newconf.pixo = s->config.pixo;
> +    newconf.alpha = s->config.alpha;
> +
>      bcm2835_fb_validate_config(&newconf);
>  
>      pitch = bcm2835_fb_get_pitch(&newconf);
>