[Spice-devel] [PATCH spice-common] canvas: Prevent some error compiling spice-gtk

[Frediano Ziglio]

> 
> On 01/23/2018 01:27 PM, Frediano Ziglio wrote:
> > ping
> > 
> > Currently master is failing to compiler, see
> > https://gitlab.com/spice/spice-gtk/-/jobs
> > 
> > This is supposed to be the patch in spice-gtk that take the spice-common
> > changes:
> > https://lists.freedesktop.org/archives/spice-devel/2018-January/041472.html
> > 
> > Frediano
> > 
> >>
> >> Due to different warning setting some GCC reports:
> >>
> >> In file included from ../spice-common/common/sw_canvas.c:27:0,
> >>                   from client_sw_canvas.c:20:
> >> ../spice-common/common/canvas_base.c: In function ‘canvas_get_lz’:
> >> ../spice-common/common/canvas_base.c:768:13: error: ‘palette’ may be used
> >> uninitialized in this function [-Werror=maybe-uninitialized]
> >>               free(palette);
> >>               ^~~~~~~~~~~~~
> >> ../spice-common/common/canvas_base.c:764:9: error: variable ‘free_palette’
> >> might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
> >>       int free_palette = FALSE;
> >>           ^~~~~~~~~~~~
> >> cc1: all warnings being treated as errors
> >>
> >> Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> >> ---
> >>   common/canvas_base.c | 8 +++++---
> >>   1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/common/canvas_base.c b/common/canvas_base.c
> >> index 42f0eea..90d1d38 100644
> >> --- a/common/canvas_base.c
> >> +++ b/common/canvas_base.c
> >> @@ -754,14 +754,14 @@ static pixman_image_t *canvas_get_lz(CanvasBase
> >> *canvas, SpiceImage *image,
> >>       uint8_t    *decomp_buf = NULL;
> >>       pixman_format_code_t pixman_format;
> >>       LzImageType type, as_type;
> >> -    SpicePalette *palette;
> >> +    SpicePalette *palette = NULL;
> >>       int n_comp_pixels;
> >>       int width;
> >>       int height;
> >>       int top_down;
> >>       int stride_encoded;
> >>       int stride;
> >> -    int free_palette = FALSE;
> >> +    volatile int free_palette = FALSE;
> 
> Hi Frediano,
> Why does free_palette have to be volatile ?

Is quite complicated, has to do with life time optimizations
and internal compiler details.
Basically the compiler is not much sure if free_palette can
be optimized out but the pointer is still alive.
Using volatile fix this logic.
I suppose is more a "maybe is a disaster" than a real issue.
volatile make sure the variable is life (and the pointer to
it too) while the jmpbuf can be used.

> Isn't it enough to initialize it ?
> What's special with free_palette compared to other
> variables ?
> 

No idea exactly. Some compiler version reports the warning
others not.
But in this case looking at how is used and how many time
is not a big issue (cannot be optimized out much or should
not and cannot be put in a register).

> You may want to remove the free_palette = FALSE; line that
> exists few lines below in the code (not shown here).
> 

Sorry, I cannot find this line you are mentioning.

> Do we need to add an if (free_palette) free() to
> the if setjmp() block ?
> 

Yes, potentially longjmp can occur after the palette is
allocated and you want to free it to avoid the leak (fixed
by "canvas: Remove possible leak on LZ decompression failure").

> Thanks,
>      Uri.
> 
> >>   
> >>       if (setjmp(lz_data->jmp_env)) {
> >>           if (free_palette)  {
> >> @@ -781,7 +781,9 @@ static pixman_image_t *canvas_get_lz(CanvasBase
> >> *canvas,
> >> SpiceImage *image,
> >>           spice_return_val_if_fail(image->u.lz_plt.data->num_chunks == 1,
> >>           NULL); /* TODO: Handle chunks */
> >>           comp_buf = image->u.lz_plt.data->chunk[0].data;
> >>           comp_size = image->u.lz_plt.data->chunk[0].len;
> >> -        palette = canvas_get_localized_palette(canvas,
> >> image->u.lz_plt.palette, image->u.lz_plt.palette_id,
> >> image->u.lz_plt.flags,
> >> &free_palette);
> >> +        palette = canvas_get_localized_palette(canvas,
> >> image->u.lz_plt.palette,
> >> +
> >> image->u.lz_plt.palette_id,
> >> image->u.lz_plt.flags,
> >> +                                               (int*) &free_palette);
> >>       } else {
> >>           spice_warn_if_reached();
> >>           return NULL;
> 

Frediano