[PATCH 0/1] MR4530: d3drm: Set image->palette only once in d3drm_image_palettise (GCC).
GCC 13 thinks that there is a use-after-free here and warns about it. We actually don't need to check the return value of realloc here at all because shrinking an allocation is guaranteed to succeed. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4530
From: Alex Henrie <alexhenrie24(a)gmail.com> GCC 13 thinks that there is a use-after-free here and warns about it. We actually don't need to check the return value of realloc here at all because shrinking an allocation is guaranteed to succeed. --- dlls/d3drm/texture.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/d3drm/texture.c b/dlls/d3drm/texture.c index f0983d53e24..b06d68cc881 100644 --- a/dlls/d3drm/texture.c +++ b/dlls/d3drm/texture.c @@ -153,8 +153,7 @@ static BOOL d3drm_image_palettise(D3DRMIMAGE *image, unsigned char *src_data, image->green_mask = 0xff; image->blue_mask = 0xff; image->palette_size = colour_count; - if (!(image->palette = realloc(palette, colour_count * sizeof(*palette)))) - image->palette = palette; + image->palette = realloc(palette, colour_count * sizeof(*palette)); return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4530
We actually don't need to check the return value of realloc here at all because shrinking an allocation is guaranteed to succeed.
I'm not sure that's guaranteed, actually (e.g. the heap allocator might want to reallocate anyway if the new size belongs to a different bucket). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4530#note_54111
Matteo Bruni (@Mystral) commented about dlls/d3drm/texture.c:
image->green_mask = 0xff; image->blue_mask = 0xff; image->palette_size = colour_count; - if (!(image->palette = realloc(palette, colour_count * sizeof(*palette)))) - image->palette = palette; + image->palette = realloc(palette, colour_count * sizeof(*palette));
So this also avoids the warning here: ```suggestion:-0+0 image->palette = palette; if ((palette = realloc(palette, colour_count * sizeof(*palette)))) image->palette = palette; ``` I'm leaving it up to the reviewers to decide if we want to be pedantic like this :grinning: -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4530#note_54112
On Tue Nov 28 16:18:08 2023 +0000, Matteo Bruni wrote:
So this also avoids the warning here: ```suggestion:-0+0 image->palette = palette; if ((palette = realloc(palette, colour_count * sizeof(*palette)))) image->palette = palette; ``` I'm leaving it up to the reviewers to decide if we want to be pedantic like this :grinning: I prefer this version, frankly (and I find it clearer than the current code anyway), though I'm also struggling to see why GCC complains about the one but not the other?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4530#note_54131
On Tue Nov 28 19:27:08 2023 +0000, Zebediah Figura wrote:
I prefer this version, frankly (and I find it clearer than the current code anyway), though I'm also struggling to see why GCC complains about the one but not the other? To be clear, do you mean that you prefer Matteo's version, or the one I originally proposed?
GCC apparently thinks that colour_count might be 0, which means that realloc might free palette and return NULL. It's not smart enough to warn about the hypothetically dangling pointer that was saved to image->palette in Matteo's version because the pointer is not dereferenced in the immediate context, and even if it were dereferenced immediately, GCC still only warns when compiling with -fanalyzer. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4530#note_54139
On Tue Nov 28 20:30:48 2023 +0000, Alex Henrie wrote:
To be clear, do you mean that you prefer Matteo's version, or the one I originally proposed? GCC apparently thinks that colour_count might be 0, which means that realloc might free palette and return NULL. It's not smart enough to warn about the hypothetically dangling pointer that was saved to image->palette in Matteo's version because the pointer is not dereferenced in the immediate context, and even if it were dereferenced immediately, GCC still only warns when compiling with -fanalyzer. Yes, I prefer Matteo's diff.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4530#note_54140
participants (4)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Matteo Bruni (＠Mystral)
-
Zebediah Figura (＠zfigura)