From ec58a817ef66efc23d7d6e964844317673b23ead Mon Sep 17 00:00:00 2001 From: ulatekh Date: Wed, 5 Oct 2022 19:26:09 -0700 Subject: [PATCH] Fixes made in response to running a static code analyzer under MS Windows. Most of these are probably harmless, but the changes to SDL_immdevice.c and SDL_pixels.c appear to have fixed genuine bugs. SDL_audiocvt.c: By separating the calculation of the divisor, I got rid of the suspicion that dividing a double by an integer led to loss of precision. SDL_immdevice.c: Added a missing test, one that could have otherwise led to dereferencing a null pointer. SDL_events.c, SDL_gamecontroller.c, SDL_joystick.c, SDL_malloc.c, SDL_video.c: Made it clear the return values weren't used. SDL_hidapi_shield.c: The size is zero, so nothing bad would have happened, but the SDL_memset() was still being given an address outside of the array's range. SDL_dinputjoystick.c: Initialize local data, just in case IDirectInputDevice8_GetProperty() isn't guaranteed to write to it. SDL_render_sw.c: drawstate.viewport could be null (as seen on line 691). SDL.c: SDL_MostSignificantBitIndex32() could return -1, though I don't know if you want to cope with that (what I did) or SDL_assert() that it can't happen. SDL_hints.c: Replaced boolean tests on pointer values with comparisons to NULL. SDL_pixels.c: Looks like the switch is genuinely missing a break! SDL_rect_impl.h: The MacOS static checker pointed out issues with the X comparisons that were handled by assertions; I added assertions for the Y comparisons. SDL_yuv.c, SDL_windowskeyboard.c, SDL_windowswindow.c: Checked error-result returns. --- src/SDL.c | 15 ++++++++------- src/SDL_hints.c | 6 +++--- src/audio/SDL_audiocvt.c | 3 ++- src/core/windows/SDL_immdevice.c | 4 +++- src/events/SDL_events.c | 12 ++++++------ src/joystick/SDL_gamecontroller.c | 2 +- src/joystick/SDL_joystick.c | 2 +- src/joystick/hidapi/SDL_hidapi_shield.c | 3 ++- src/joystick/windows/SDL_dinputjoystick.c | 1 + src/render/software/SDL_render_sw.c | 14 +++++++------- src/stdlib/SDL_malloc.c | 2 +- src/video/SDL_pixels.c | 1 + src/video/SDL_rect_impl.h | 2 ++ src/video/SDL_video.c | 8 ++++---- src/video/SDL_yuv.c | 7 ++++--- src/video/windows/SDL_windowskeyboard.c | 10 +++++++++- src/video/windows/SDL_windowswindow.c | 12 ++++++++---- 17 files changed, 63 insertions(+), 41 deletions(-) diff --git a/src/SDL.c b/src/SDL.c index 67db48c3b..42ebc2cfc 100644 --- a/src/SDL.c +++ b/src/SDL.c @@ -125,8 +125,9 @@ static void SDL_PrivateSubsystemRefCountIncr(Uint32 subsystem) { int subsystem_index = SDL_MostSignificantBitIndex32(subsystem); - SDL_assert(SDL_SubsystemRefCount[subsystem_index] < 255); - ++SDL_SubsystemRefCount[subsystem_index]; + SDL_assert(subsystem_index < 0 || SDL_SubsystemRefCount[subsystem_index] < 255); + if (subsystem_index >= 0) + ++SDL_SubsystemRefCount[subsystem_index]; } /* Private helper to decrement a subsystem's ref counter. */ @@ -134,7 +135,7 @@ static void SDL_PrivateSubsystemRefCountDecr(Uint32 subsystem) { int subsystem_index = SDL_MostSignificantBitIndex32(subsystem); - if (SDL_SubsystemRefCount[subsystem_index] > 0) { + if (subsystem_index >= 0 && SDL_SubsystemRefCount[subsystem_index] > 0) { --SDL_SubsystemRefCount[subsystem_index]; } } @@ -144,22 +145,22 @@ static SDL_bool SDL_PrivateShouldInitSubsystem(Uint32 subsystem) { int subsystem_index = SDL_MostSignificantBitIndex32(subsystem); - SDL_assert(SDL_SubsystemRefCount[subsystem_index] < 255); - return (SDL_SubsystemRefCount[subsystem_index] == 0) ? SDL_TRUE : SDL_FALSE; + SDL_assert(subsystem_index < 0 || SDL_SubsystemRefCount[subsystem_index] < 255); + return (subsystem_index >= 0 && SDL_SubsystemRefCount[subsystem_index] == 0) ? SDL_TRUE : SDL_FALSE; } /* Private helper to check if a system needs to be quit. */ static SDL_bool SDL_PrivateShouldQuitSubsystem(Uint32 subsystem) { int subsystem_index = SDL_MostSignificantBitIndex32(subsystem); - if (SDL_SubsystemRefCount[subsystem_index] == 0) { + if (subsystem_index >= 0 && SDL_SubsystemRefCount[subsystem_index] == 0) { return SDL_FALSE; } /* If we're in SDL_Quit, we shut down every subsystem, even if refcount * isn't zero. */ - return (SDL_SubsystemRefCount[subsystem_index] == 1 || SDL_bInMainQuit) ? SDL_TRUE : SDL_FALSE; + return ((subsystem_index >= 0 && SDL_SubsystemRefCount[subsystem_index] == 1) || SDL_bInMainQuit) ? SDL_TRUE : SDL_FALSE; } void diff --git a/src/SDL_hints.c b/src/SDL_hints.c index 4f2f5f61d..4341f8d20 100644 --- a/src/SDL_hints.c +++ b/src/SDL_hints.c @@ -112,7 +112,7 @@ SDL_ResetHint(const char *name) if (SDL_strcmp(name, hint->name) == 0) { if ((env == NULL && hint->value != NULL) || (env != NULL && hint->value == NULL) || - (env && SDL_strcmp(env, hint->value) != 0)) { + (env != NULL && SDL_strcmp(env, hint->value) != 0)) { for (entry = hint->callbacks; entry; ) { /* Save the next entry in case this one is deleted */ SDL_HintWatch *next = entry->next; @@ -140,7 +140,7 @@ SDL_ResetHints(void) env = SDL_getenv(hint->name); if ((env == NULL && hint->value != NULL) || (env != NULL && hint->value == NULL) || - (env && SDL_strcmp(env, hint->value) != 0)) { + (env != NULL && SDL_strcmp(env, hint->value) != 0)) { for (entry = hint->callbacks; entry; ) { /* Save the next entry in case this one is deleted */ SDL_HintWatch *next = entry->next; @@ -169,7 +169,7 @@ SDL_GetHint(const char *name) env = SDL_getenv(name); for (hint = SDL_hints; hint; hint = hint->next) { if (SDL_strcmp(name, hint->name) == 0) { - if (!env || hint->priority == SDL_HINT_OVERRIDE) { + if (env == NULL || hint->priority == SDL_HINT_OVERRIDE) { return hint->value; } break; diff --git a/src/audio/SDL_audiocvt.c b/src/audio/SDL_audiocvt.c index 0884fa373..196013e11 100644 --- a/src/audio/SDL_audiocvt.c +++ b/src/audio/SDL_audiocvt.c @@ -403,7 +403,8 @@ SDL_BuildAudioTypeCVTFromFloat(SDL_AudioCVT *cvt, const SDL_AudioFormat dst_fmt) cvt->len_mult *= mult; cvt->len_ratio *= mult; } else if (src_bitsize > dst_bitsize) { - cvt->len_ratio /= (src_bitsize / dst_bitsize); + const int div = (src_bitsize / dst_bitsize); + cvt->len_ratio /= div; } retval = 1; /* added a converter. */ } diff --git a/src/core/windows/SDL_immdevice.c b/src/core/windows/SDL_immdevice.c index 0a00a6d02..335596d95 100644 --- a/src/core/windows/SDL_immdevice.c +++ b/src/core/windows/SDL_immdevice.c @@ -399,7 +399,9 @@ static int SDLCALL sort_endpoints(const void *_a, const void *_b) { LPWSTR a = ((const EndpointItem *)_a)->devid; LPWSTR b = ((const EndpointItem *)_b)->devid; - if (!a && b) { + if (!a && !b) { + return 0; + } else if (!a && b) { return -1; } else if (a && !b) { return 1; diff --git a/src/events/SDL_events.c b/src/events/SDL_events.c index 52b72cd22..9ba5e44b8 100644 --- a/src/events/SDL_events.c +++ b/src/events/SDL_events.c @@ -147,7 +147,7 @@ SDL_AutoUpdateSensorsChanged(void *userdata, const char *name, const char *oldVa static void SDLCALL SDL_PollSentinelChanged(void *userdata, const char *name, const char *oldValue, const char *hint) { - SDL_EventState(SDL_POLLSENTINEL, SDL_GetStringBoolean(hint, SDL_TRUE) ? SDL_ENABLE : SDL_DISABLE); + (void)SDL_EventState(SDL_POLLSENTINEL, SDL_GetStringBoolean(hint, SDL_TRUE) ? SDL_ENABLE : SDL_DISABLE); } /** @@ -566,12 +566,12 @@ SDL_StartEventLoop(void) #endif /* !SDL_THREADS_DISABLED */ /* Process most event types */ - SDL_EventState(SDL_TEXTINPUT, SDL_DISABLE); - SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE); - SDL_EventState(SDL_SYSWMEVENT, SDL_DISABLE); + (void)SDL_EventState(SDL_TEXTINPUT, SDL_DISABLE); + (void)SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE); + (void)SDL_EventState(SDL_SYSWMEVENT, SDL_DISABLE); #if 0 /* Leave these events enabled so apps can respond to items being dragged onto them at startup */ - SDL_EventState(SDL_DROPFILE, SDL_DISABLE); - SDL_EventState(SDL_DROPTEXT, SDL_DISABLE); + (void)SDL_EventState(SDL_DROPFILE, SDL_DISABLE); + (void)SDL_EventState(SDL_DROPTEXT, SDL_DISABLE); #endif SDL_EventQ.active = SDL_TRUE; diff --git a/src/joystick/SDL_gamecontroller.c b/src/joystick/SDL_gamecontroller.c index 2bb97657b..26979a2ff 100644 --- a/src/joystick/SDL_gamecontroller.c +++ b/src/joystick/SDL_gamecontroller.c @@ -3009,7 +3009,7 @@ SDL_GameControllerEventState(int state) break; default: for (i = 0; i < SDL_arraysize(event_list); ++i) { - SDL_EventState(event_list[i], state); + (void)SDL_EventState(event_list[i], state); } break; } diff --git a/src/joystick/SDL_joystick.c b/src/joystick/SDL_joystick.c index c6d2bf393..4b9558145 100644 --- a/src/joystick/SDL_joystick.c +++ b/src/joystick/SDL_joystick.c @@ -1798,7 +1798,7 @@ SDL_JoystickEventState(int state) break; default: for (i = 0; i < SDL_arraysize(event_list); ++i) { - SDL_EventState(event_list[i], state); + (void)SDL_EventState(event_list[i], state); } break; } diff --git a/src/joystick/hidapi/SDL_hidapi_shield.c b/src/joystick/hidapi/SDL_hidapi_shield.c index 1d54676b8..557ca45ab 100644 --- a/src/joystick/hidapi/SDL_hidapi_shield.c +++ b/src/joystick/hidapi/SDL_hidapi_shield.c @@ -169,7 +169,8 @@ HIDAPI_DriverShield_SendCommand(SDL_HIDAPI_Device *device, Uint8 cmd, const void } /* Zero unused data in the payload */ - SDL_memset(&cmd_pkt.payload[size], 0, sizeof(cmd_pkt.payload) - size); + if (size != sizeof(cmd_pkt.payload)) + SDL_memset(&cmd_pkt.payload[size], 0, sizeof(cmd_pkt.payload) - size); if (SDL_HIDAPI_SendRumbleAndUnlock(device, (Uint8*)&cmd_pkt, sizeof(cmd_pkt)) != sizeof(cmd_pkt)) { return SDL_SetError("Couldn't send command packet"); diff --git a/src/joystick/windows/SDL_dinputjoystick.c b/src/joystick/windows/SDL_dinputjoystick.c index 2f0605370..9c5a46bd0 100644 --- a/src/joystick/windows/SDL_dinputjoystick.c +++ b/src/joystick/windows/SDL_dinputjoystick.c @@ -329,6 +329,7 @@ QueryDeviceInfo(LPDIRECTINPUTDEVICE8 device, Uint16* vendor_id, Uint16* product_ dipdw.diph.dwHeaderSize = sizeof(dipdw.diph); dipdw.diph.dwObj = 0; dipdw.diph.dwHow = DIPH_DEVICE; + dipdw.dwData = 0; if (FAILED(IDirectInputDevice8_GetProperty(device, DIPROP_VIDPID, &dipdw.diph))) { return SDL_FALSE; diff --git a/src/render/software/SDL_render_sw.c b/src/render/software/SDL_render_sw.c index 874f92b33..6ba77d46e 100644 --- a/src/render/software/SDL_render_sw.c +++ b/src/render/software/SDL_render_sw.c @@ -733,7 +733,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic SetDrawState(surface, &drawstate); /* Apply viewport */ - if (drawstate.viewport->x || drawstate.viewport->y) { + if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) { int i; for (i = 0; i < count; i++) { verts[i].x += drawstate.viewport->x; @@ -760,7 +760,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic SetDrawState(surface, &drawstate); /* Apply viewport */ - if (drawstate.viewport->x || drawstate.viewport->y) { + if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) { int i; for (i = 0; i < count; i++) { verts[i].x += drawstate.viewport->x; @@ -787,7 +787,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic SetDrawState(surface, &drawstate); /* Apply viewport */ - if (drawstate.viewport->x || drawstate.viewport->y) { + if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) { int i; for (i = 0; i < count; i++) { verts[i].x += drawstate.viewport->x; @@ -815,7 +815,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic PrepTextureForCopy(cmd); /* Apply viewport */ - if (drawstate.viewport->x || drawstate.viewport->y) { + if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) { dstrect->x += drawstate.viewport->x; dstrect->y += drawstate.viewport->y; } @@ -873,7 +873,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic PrepTextureForCopy(cmd); /* Apply viewport */ - if (drawstate.viewport->x || drawstate.viewport->y) { + if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) { copydata->dstrect.x += drawstate.viewport->x; copydata->dstrect.y += drawstate.viewport->y; } @@ -901,7 +901,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic PrepTextureForCopy(cmd); /* Apply viewport */ - if (drawstate.viewport->x || drawstate.viewport->y) { + if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) { SDL_Point vp; vp.x = drawstate.viewport->x; vp.y = drawstate.viewport->y; @@ -924,7 +924,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic GeometryFillData *ptr = (GeometryFillData *) verts; /* Apply viewport */ - if (drawstate.viewport->x || drawstate.viewport->y) { + if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) { SDL_Point vp; vp.x = drawstate.viewport->x; vp.y = drawstate.viewport->y; diff --git a/src/stdlib/SDL_malloc.c b/src/stdlib/SDL_malloc.c index ebafb66cf..de02b5eef 100644 --- a/src/stdlib/SDL_malloc.c +++ b/src/stdlib/SDL_malloc.c @@ -2580,7 +2580,7 @@ init_mparams(void) #else /* (FOOTERS && !INSECURE) */ s = (size_t) 0x58585858U; #endif /* (FOOTERS && !INSECURE) */ - ACQUIRE_MAGIC_INIT_LOCK(); + (void)ACQUIRE_MAGIC_INIT_LOCK(); if (mparams.magic == 0) { mparams.magic = s; /* Set up lock for main malloc area */ diff --git a/src/video/SDL_pixels.c b/src/video/SDL_pixels.c index 1646d844f..654453278 100644 --- a/src/video/SDL_pixels.c +++ b/src/video/SDL_pixels.c @@ -436,6 +436,7 @@ SDL_MasksToPixelFormatEnum(int bpp, Uint32 Rmask, Uint32 Gmask, Uint32 Bmask, return SDL_PIXELFORMAT_RGB24; #endif } + break; case 32: if (Rmask == 0) { return SDL_PIXELFORMAT_RGB888; diff --git a/src/video/SDL_rect_impl.h b/src/video/SDL_rect_impl.h index 993bb8eb0..26a54484a 100644 --- a/src/video/SDL_rect_impl.h +++ b/src/video/SDL_rect_impl.h @@ -400,9 +400,11 @@ SDL_INTERSECTRECTANDLINE(const RECTTYPE * rect, SCALARTYPE *X1, SCALARTYPE *Y1, outcode1 = COMPUTEOUTCODE(rect, x, y); } else { if (outcode2 & CODE_TOP) { + SDL_assert(y2 != y1); /* if equal: division by zero. */ y = recty1; x = x1 + ((x2 - x1) * (y - y1)) / (y2 - y1); } else if (outcode2 & CODE_BOTTOM) { + SDL_assert(y2 != y1); /* if equal: division by zero. */ y = recty2; x = x1 + ((x2 - x1) * (y - y1)) / (y2 - y1); } else if (outcode2 & CODE_LEFT) { diff --git a/src/video/SDL_video.c b/src/video/SDL_video.c index c75b026d9..1ac9165ad 100644 --- a/src/video/SDL_video.c +++ b/src/video/SDL_video.c @@ -4337,8 +4337,8 @@ SDL_StartTextInput(void) SDL_Window *window; /* First, enable text events */ - SDL_EventState(SDL_TEXTINPUT, SDL_ENABLE); - SDL_EventState(SDL_TEXTEDITING, SDL_ENABLE); + (void)SDL_EventState(SDL_TEXTINPUT, SDL_ENABLE); + (void)SDL_EventState(SDL_TEXTEDITING, SDL_ENABLE); /* Then show the on-screen keyboard, if any */ window = SDL_GetFocusWindow(); @@ -4393,8 +4393,8 @@ SDL_StopTextInput(void) } /* Finally disable text events */ - SDL_EventState(SDL_TEXTINPUT, SDL_DISABLE); - SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE); + (void)SDL_EventState(SDL_TEXTINPUT, SDL_DISABLE); + (void)SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE); } void diff --git a/src/video/SDL_yuv.c b/src/video/SDL_yuv.c index 50395ff48..b163216b9 100644 --- a/src/video/SDL_yuv.c +++ b/src/video/SDL_yuv.c @@ -601,9 +601,10 @@ SDL_ConvertPixels_ARGB8888_to_YUV(int width, int height, const void *src, int sr Uint8 *plane_interleaved_uv; Uint32 y_stride, uv_stride, y_skip, uv_skip; - GetYUVPlanes(width, height, dst_format, dst, dst_pitch, - (const Uint8 **)&plane_y, (const Uint8 **)&plane_u, (const Uint8 **)&plane_v, - &y_stride, &uv_stride); + if (GetYUVPlanes(width, height, dst_format, dst, dst_pitch, + (const Uint8 **)&plane_y, (const Uint8 **)&plane_u, (const Uint8 **)&plane_v, + &y_stride, &uv_stride) != 0) + return -1; plane_interleaved_uv = (plane_y + height * y_stride); y_skip = (y_stride - width); diff --git a/src/video/windows/SDL_windowskeyboard.c b/src/video/windows/SDL_windowskeyboard.c index 4f288c77a..3a9988deb 100644 --- a/src/video/windows/SDL_windowskeyboard.c +++ b/src/video/windows/SDL_windowskeyboard.c @@ -389,13 +389,21 @@ WIN_ShouldShowNativeUI() static void IME_Init(SDL_VideoData *videodata, HWND hwnd) { + HRESULT hResult = S_OK; + if (videodata->ime_initialized) return; videodata->ime_hwnd_main = hwnd; if (SUCCEEDED(WIN_CoInitialize())) { videodata->ime_com_initialized = SDL_TRUE; - CoCreateInstance(&CLSID_TF_ThreadMgr, NULL, CLSCTX_INPROC_SERVER, &IID_ITfThreadMgr, (LPVOID *)&videodata->ime_threadmgr); + hResult = CoCreateInstance(&CLSID_TF_ThreadMgr, NULL, CLSCTX_INPROC_SERVER, &IID_ITfThreadMgr, (LPVOID *)&videodata->ime_threadmgr); + if (hResult != S_OK) + { + videodata->ime_available = SDL_FALSE; + SDL_SetError("CoCreateInstance() failed, HRESULT is %08X", (unsigned int)hResult); + return; + } } videodata->ime_initialized = SDL_TRUE; videodata->ime_himm32 = SDL_LoadObject("imm32.dll"); diff --git a/src/video/windows/SDL_windowswindow.c b/src/video/windows/SDL_windowswindow.c index 488b6fe1a..d74f1dda7 100644 --- a/src/video/windows/SDL_windowswindow.c +++ b/src/video/windows/SDL_windowswindow.c @@ -739,15 +739,18 @@ WIN_GetWindowBordersSize(_THIS, SDL_Window * window, int *top, int *left, int *b /* rcClient stores the size of the inner window, while rcWindow stores the outer size relative to the top-left * screen position; so the top/left values of rcClient are always {0,0} and bottom/right are {height,width} */ - GetClientRect(hwnd, &rcClient); - GetWindowRect(hwnd, &rcWindow); + if (!GetClientRect(hwnd, &rcClient)) + SDL_SetError("GetClientRect() failed, error %08X", (unsigned int)GetLastError()); + if (!GetWindowRect(hwnd, &rcWindow)) + SDL_SetError("GetWindowRect() failed, error %08X", (unsigned int)GetLastError()); /* convert the top/left values to make them relative to * the window; they will end up being slightly negative */ ptDiff.y = rcWindow.top; ptDiff.x = rcWindow.left; - ScreenToClient(hwnd, &ptDiff); + if (!ScreenToClient(hwnd, &ptDiff)) + SDL_SetError("ScreenToClient() failed, error %08X", (unsigned int)GetLastError()); rcWindow.top = ptDiff.y; rcWindow.left = ptDiff.x; @@ -757,7 +760,8 @@ WIN_GetWindowBordersSize(_THIS, SDL_Window * window, int *top, int *left, int *b ptDiff.y = rcWindow.bottom; ptDiff.x = rcWindow.right; - ScreenToClient(hwnd, &ptDiff); + if (!ScreenToClient(hwnd, &ptDiff)) + SDL_SetError("ScreenToClient() failed, error %08X", (unsigned int)GetLastError()); rcWindow.bottom = ptDiff.y; rcWindow.right = ptDiff.x;