[PATCH 0/1] MR3846: winmm: Handle memory allocation failure in MMDRV_InitPerType (cppcheck).
From: Alex Henrie <alexhenrie24(a)gmail.com> --- dlls/winmm/lolvldrv.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/winmm/lolvldrv.c b/dlls/winmm/lolvldrv.c index e664021dc64..7977fcfba5f 100644 --- a/dlls/winmm/lolvldrv.c +++ b/dlls/winmm/lolvldrv.c @@ -374,6 +374,7 @@ static BOOL MMDRV_InitPerType(LPWINE_MM_DRIVER lpDrv, UINT type, UINT wMsg) /* realloc translation table */ mem = llTypes[type].lpMlds ? llTypes[type].lpMlds - 1 : NULL; mem = realloc(mem, sizeof(WINE_MLD) * (llTypes[type].wMaxId + 1)); + if (!mem) return FALSE; llTypes[type].lpMlds = mem + 1; /* re-build the translation table */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3846
This merge request was approved by Huw Davies. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3846
I've approved this and !3847, but generally these sort of 'fixes' aren't particularly useful. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3846#note_45473
I've approved this and !3847, but generally these sort of 'fixes' aren't particularly useful.
I wonder if we should establish some sort of policy around this, because I've seen different maintainers take different positions. (And if checking for allocation failure isn't useful, I don't really see why these patches should be accepted regardless; better not to clutter the code I'd think.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3846#note_45546
On Fri Sep 15 17:33:31 2023 +0000, Zebediah Figura wrote:
I've approved this and !3847, but generally these sort of 'fixes' aren't particularly useful. I wonder if we should establish some sort of policy around this, because I've seen different maintainers take different positions. (And if checking for allocation failure isn't useful, I don't really see why these patches should be accepted regardless; better not to clutter the code I'd think.) Having a formal memory error handling policy could be helpful. For what it's worth, I've only been submitting patches like this if both a static analyzer warns about it and the API has a clear way to handle an out-of-memory error.
By the way, there are several more important patches of mine waiting in the queue. Looking at static analyzer warnings is just a side project while I wait for feedback on the others. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3846#note_45548
I wonder if we should establish some sort of policy around this, because I've seen different maintainers take different positions. (And if checking for allocation failure isn't useful, I don't really see why these patches should be accepted regardless; better not to clutter the code I'd think.)
There can of course be good reasons to check for allocation failures - it depends on the situation. Further, if new code is written that includes checks then I don't care and that could to some extent be deemed personal choice. What we probably should avoid is MRs that add unnecessary checks to existing code - we all have better things to spend our time on. I did accept these MRs because I didn't really want to get into a debate about this... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3846#note_45550
participants (4)
-
Alex Henrie -
Alex Henrie (＠alexhenrie)
-
Huw Davies (＠huw)
-
Zebediah Figura (＠zfigura)