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.
From: Alex Henrie alexhenrie24@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; }
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).
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:
On Tue Nov 28 16:18:08 2023 +0000, Matteo Bruni wrote:
So this also avoids the warning here:
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?
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.
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.
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Matteo Bruni (＠Mystral)
-
Zebediah Figura (＠zfigura)