Skip to content

Commit ec58a81

Browse files
ulatekhicculus
ulatekh
authored and
icculus
committed
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.
1 parent 7ebdae5 commit ec58a81

17 files changed

+63
-41
lines changed

‎src/SDL.c

+8-7
Original file line numberDiff line numberDiff line change
@@ -125,16 +125,17 @@ static void
125125
SDL_PrivateSubsystemRefCountIncr(Uint32 subsystem)
126126
{
127127
int subsystem_index = SDL_MostSignificantBitIndex32(subsystem);
128-
SDL_assert(SDL_SubsystemRefCount[subsystem_index] < 255);
129-
++SDL_SubsystemRefCount[subsystem_index];
128+
SDL_assert(subsystem_index < 0 || SDL_SubsystemRefCount[subsystem_index] < 255);
129+
if (subsystem_index >= 0)
130+
++SDL_SubsystemRefCount[subsystem_index];
130131
}
131132

132133
/* Private helper to decrement a subsystem's ref counter. */
133134
static void
134135
SDL_PrivateSubsystemRefCountDecr(Uint32 subsystem)
135136
{
136137
int subsystem_index = SDL_MostSignificantBitIndex32(subsystem);
137-
if (SDL_SubsystemRefCount[subsystem_index] > 0) {
138+
if (subsystem_index >= 0 && SDL_SubsystemRefCount[subsystem_index] > 0) {
138139
--SDL_SubsystemRefCount[subsystem_index];
139140
}
140141
}
@@ -144,22 +145,22 @@ static SDL_bool
144145
SDL_PrivateShouldInitSubsystem(Uint32 subsystem)
145146
{
146147
int subsystem_index = SDL_MostSignificantBitIndex32(subsystem);
147-
SDL_assert(SDL_SubsystemRefCount[subsystem_index] < 255);
148-
return (SDL_SubsystemRefCount[subsystem_index] == 0) ? SDL_TRUE : SDL_FALSE;
148+
SDL_assert(subsystem_index < 0 || SDL_SubsystemRefCount[subsystem_index] < 255);
149+
return (subsystem_index >= 0 && SDL_SubsystemRefCount[subsystem_index] == 0) ? SDL_TRUE : SDL_FALSE;
149150
}
150151

151152
/* Private helper to check if a system needs to be quit. */
152153
static SDL_bool
153154
SDL_PrivateShouldQuitSubsystem(Uint32 subsystem) {
154155
int subsystem_index = SDL_MostSignificantBitIndex32(subsystem);
155-
if (SDL_SubsystemRefCount[subsystem_index] == 0) {
156+
if (subsystem_index >= 0 && SDL_SubsystemRefCount[subsystem_index] == 0) {
156157
return SDL_FALSE;
157158
}
158159

159160
/* If we're in SDL_Quit, we shut down every subsystem, even if refcount
160161
* isn't zero.
161162
*/
162-
return (SDL_SubsystemRefCount[subsystem_index] == 1 || SDL_bInMainQuit) ? SDL_TRUE : SDL_FALSE;
163+
return ((subsystem_index >= 0 && SDL_SubsystemRefCount[subsystem_index] == 1) || SDL_bInMainQuit) ? SDL_TRUE : SDL_FALSE;
163164
}
164165

165166
void

‎src/SDL_hints.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ SDL_ResetHint(const char *name)
112112
if (SDL_strcmp(name, hint->name) == 0) {
113113
if ((env == NULL && hint->value != NULL) ||
114114
(env != NULL && hint->value == NULL) ||
115-
(env && SDL_strcmp(env, hint->value) != 0)) {
115+
(env != NULL && SDL_strcmp(env, hint->value) != 0)) {
116116
for (entry = hint->callbacks; entry; ) {
117117
/* Save the next entry in case this one is deleted */
118118
SDL_HintWatch *next = entry->next;
@@ -140,7 +140,7 @@ SDL_ResetHints(void)
140140
env = SDL_getenv(hint->name);
141141
if ((env == NULL && hint->value != NULL) ||
142142
(env != NULL && hint->value == NULL) ||
143-
(env && SDL_strcmp(env, hint->value) != 0)) {
143+
(env != NULL && SDL_strcmp(env, hint->value) != 0)) {
144144
for (entry = hint->callbacks; entry; ) {
145145
/* Save the next entry in case this one is deleted */
146146
SDL_HintWatch *next = entry->next;
@@ -169,7 +169,7 @@ SDL_GetHint(const char *name)
169169
env = SDL_getenv(name);
170170
for (hint = SDL_hints; hint; hint = hint->next) {
171171
if (SDL_strcmp(name, hint->name) == 0) {
172-
if (!env || hint->priority == SDL_HINT_OVERRIDE) {
172+
if (env == NULL || hint->priority == SDL_HINT_OVERRIDE) {
173173
return hint->value;
174174
}
175175
break;

‎src/audio/SDL_audiocvt.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,8 @@ SDL_BuildAudioTypeCVTFromFloat(SDL_AudioCVT *cvt, const SDL_AudioFormat dst_fmt)
403403
cvt->len_mult *= mult;
404404
cvt->len_ratio *= mult;
405405
} else if (src_bitsize > dst_bitsize) {
406-
cvt->len_ratio /= (src_bitsize / dst_bitsize);
406+
const int div = (src_bitsize / dst_bitsize);
407+
cvt->len_ratio /= div;
407408
}
408409
retval = 1; /* added a converter. */
409410
}

‎src/core/windows/SDL_immdevice.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,9 @@ static int SDLCALL sort_endpoints(const void *_a, const void *_b)
399399
{
400400
LPWSTR a = ((const EndpointItem *)_a)->devid;
401401
LPWSTR b = ((const EndpointItem *)_b)->devid;
402-
if (!a && b) {
402+
if (!a && !b) {
403+
return 0;
404+
} else if (!a && b) {
403405
return -1;
404406
} else if (a && !b) {
405407
return 1;

‎src/events/SDL_events.c

+6-6
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ SDL_AutoUpdateSensorsChanged(void *userdata, const char *name, const char *oldVa
147147
static void SDLCALL
148148
SDL_PollSentinelChanged(void *userdata, const char *name, const char *oldValue, const char *hint)
149149
{
150-
SDL_EventState(SDL_POLLSENTINEL, SDL_GetStringBoolean(hint, SDL_TRUE) ? SDL_ENABLE : SDL_DISABLE);
150+
(void)SDL_EventState(SDL_POLLSENTINEL, SDL_GetStringBoolean(hint, SDL_TRUE) ? SDL_ENABLE : SDL_DISABLE);
151151
}
152152

153153
/**
@@ -566,12 +566,12 @@ SDL_StartEventLoop(void)
566566
#endif /* !SDL_THREADS_DISABLED */
567567

568568
/* Process most event types */
569-
SDL_EventState(SDL_TEXTINPUT, SDL_DISABLE);
570-
SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE);
571-
SDL_EventState(SDL_SYSWMEVENT, SDL_DISABLE);
569+
(void)SDL_EventState(SDL_TEXTINPUT, SDL_DISABLE);
570+
(void)SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE);
571+
(void)SDL_EventState(SDL_SYSWMEVENT, SDL_DISABLE);
572572
#if 0 /* Leave these events enabled so apps can respond to items being dragged onto them at startup */
573-
SDL_EventState(SDL_DROPFILE, SDL_DISABLE);
574-
SDL_EventState(SDL_DROPTEXT, SDL_DISABLE);
573+
(void)SDL_EventState(SDL_DROPFILE, SDL_DISABLE);
574+
(void)SDL_EventState(SDL_DROPTEXT, SDL_DISABLE);
575575
#endif
576576

577577
SDL_EventQ.active = SDL_TRUE;

‎src/joystick/SDL_gamecontroller.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -3009,7 +3009,7 @@ SDL_GameControllerEventState(int state)
30093009
break;
30103010
default:
30113011
for (i = 0; i < SDL_arraysize(event_list); ++i) {
3012-
SDL_EventState(event_list[i], state);
3012+
(void)SDL_EventState(event_list[i], state);
30133013
}
30143014
break;
30153015
}

‎src/joystick/SDL_joystick.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -1798,7 +1798,7 @@ SDL_JoystickEventState(int state)
17981798
break;
17991799
default:
18001800
for (i = 0; i < SDL_arraysize(event_list); ++i) {
1801-
SDL_EventState(event_list[i], state);
1801+
(void)SDL_EventState(event_list[i], state);
18021802
}
18031803
break;
18041804
}

‎src/joystick/hidapi/SDL_hidapi_shield.c

+2-1
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,8 @@ HIDAPI_DriverShield_SendCommand(SDL_HIDAPI_Device *device, Uint8 cmd, const void
169169
}
170170

171171
/* Zero unused data in the payload */
172-
SDL_memset(&cmd_pkt.payload[size], 0, sizeof(cmd_pkt.payload) - size);
172+
if (size != sizeof(cmd_pkt.payload))
173+
SDL_memset(&cmd_pkt.payload[size], 0, sizeof(cmd_pkt.payload) - size);
173174

174175
if (SDL_HIDAPI_SendRumbleAndUnlock(device, (Uint8*)&cmd_pkt, sizeof(cmd_pkt)) != sizeof(cmd_pkt)) {
175176
return SDL_SetError("Couldn't send command packet");

‎src/joystick/windows/SDL_dinputjoystick.c

+1
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,7 @@ QueryDeviceInfo(LPDIRECTINPUTDEVICE8 device, Uint16* vendor_id, Uint16* product_
329329
dipdw.diph.dwHeaderSize = sizeof(dipdw.diph);
330330
dipdw.diph.dwObj = 0;
331331
dipdw.diph.dwHow = DIPH_DEVICE;
332+
dipdw.dwData = 0;
332333

333334
if (FAILED(IDirectInputDevice8_GetProperty(device, DIPROP_VIDPID, &dipdw.diph))) {
334335
return SDL_FALSE;

‎src/render/software/SDL_render_sw.c

+7-7
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic
733733
SetDrawState(surface, &drawstate);
734734

735735
/* Apply viewport */
736-
if (drawstate.viewport->x || drawstate.viewport->y) {
736+
if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) {
737737
int i;
738738
for (i = 0; i < count; i++) {
739739
verts[i].x += drawstate.viewport->x;
@@ -760,7 +760,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic
760760
SetDrawState(surface, &drawstate);
761761

762762
/* Apply viewport */
763-
if (drawstate.viewport->x || drawstate.viewport->y) {
763+
if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) {
764764
int i;
765765
for (i = 0; i < count; i++) {
766766
verts[i].x += drawstate.viewport->x;
@@ -787,7 +787,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic
787787
SetDrawState(surface, &drawstate);
788788

789789
/* Apply viewport */
790-
if (drawstate.viewport->x || drawstate.viewport->y) {
790+
if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) {
791791
int i;
792792
for (i = 0; i < count; i++) {
793793
verts[i].x += drawstate.viewport->x;
@@ -815,7 +815,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic
815815
PrepTextureForCopy(cmd);
816816

817817
/* Apply viewport */
818-
if (drawstate.viewport->x || drawstate.viewport->y) {
818+
if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) {
819819
dstrect->x += drawstate.viewport->x;
820820
dstrect->y += drawstate.viewport->y;
821821
}
@@ -873,7 +873,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic
873873
PrepTextureForCopy(cmd);
874874

875875
/* Apply viewport */
876-
if (drawstate.viewport->x || drawstate.viewport->y) {
876+
if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) {
877877
copydata->dstrect.x += drawstate.viewport->x;
878878
copydata->dstrect.y += drawstate.viewport->y;
879879
}
@@ -901,7 +901,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic
901901
PrepTextureForCopy(cmd);
902902

903903
/* Apply viewport */
904-
if (drawstate.viewport->x || drawstate.viewport->y) {
904+
if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) {
905905
SDL_Point vp;
906906
vp.x = drawstate.viewport->x;
907907
vp.y = drawstate.viewport->y;
@@ -924,7 +924,7 @@ SW_RunCommandQueue(SDL_Renderer * renderer, SDL_RenderCommand *cmd, void *vertic
924924
GeometryFillData *ptr = (GeometryFillData *) verts;
925925

926926
/* Apply viewport */
927-
if (drawstate.viewport->x || drawstate.viewport->y) {
927+
if (drawstate.viewport != NULL && (drawstate.viewport->x || drawstate.viewport->y)) {
928928
SDL_Point vp;
929929
vp.x = drawstate.viewport->x;
930930
vp.y = drawstate.viewport->y;

‎src/stdlib/SDL_malloc.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -2580,7 +2580,7 @@ init_mparams(void)
25802580
#else /* (FOOTERS && !INSECURE) */
25812581
s = (size_t) 0x58585858U;
25822582
#endif /* (FOOTERS && !INSECURE) */
2583-
ACQUIRE_MAGIC_INIT_LOCK();
2583+
(void)ACQUIRE_MAGIC_INIT_LOCK();
25842584
if (mparams.magic == 0) {
25852585
mparams.magic = s;
25862586
/* Set up lock for main malloc area */

‎src/video/SDL_pixels.c

+1
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,7 @@ SDL_MasksToPixelFormatEnum(int bpp, Uint32 Rmask, Uint32 Gmask, Uint32 Bmask,
436436
return SDL_PIXELFORMAT_RGB24;
437437
#endif
438438
}
439+
break;
439440
case 32:
440441
if (Rmask == 0) {
441442
return SDL_PIXELFORMAT_RGB888;

‎src/video/SDL_rect_impl.h

+2
Original file line numberDiff line numberDiff line change
@@ -400,9 +400,11 @@ SDL_INTERSECTRECTANDLINE(const RECTTYPE * rect, SCALARTYPE *X1, SCALARTYPE *Y1,
400400
outcode1 = COMPUTEOUTCODE(rect, x, y);
401401
} else {
402402
if (outcode2 & CODE_TOP) {
403+
SDL_assert(y2 != y1); /* if equal: division by zero. */
403404
y = recty1;
404405
x = x1 + ((x2 - x1) * (y - y1)) / (y2 - y1);
405406
} else if (outcode2 & CODE_BOTTOM) {
407+
SDL_assert(y2 != y1); /* if equal: division by zero. */
406408
y = recty2;
407409
x = x1 + ((x2 - x1) * (y - y1)) / (y2 - y1);
408410
} else if (outcode2 & CODE_LEFT) {

‎src/video/SDL_video.c

+4-4
Original file line numberDiff line numberDiff line change
@@ -4337,8 +4337,8 @@ SDL_StartTextInput(void)
43374337
SDL_Window *window;
43384338

43394339
/* First, enable text events */
4340-
SDL_EventState(SDL_TEXTINPUT, SDL_ENABLE);
4341-
SDL_EventState(SDL_TEXTEDITING, SDL_ENABLE);
4340+
(void)SDL_EventState(SDL_TEXTINPUT, SDL_ENABLE);
4341+
(void)SDL_EventState(SDL_TEXTEDITING, SDL_ENABLE);
43424342

43434343
/* Then show the on-screen keyboard, if any */
43444344
window = SDL_GetFocusWindow();
@@ -4393,8 +4393,8 @@ SDL_StopTextInput(void)
43934393
}
43944394

43954395
/* Finally disable text events */
4396-
SDL_EventState(SDL_TEXTINPUT, SDL_DISABLE);
4397-
SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE);
4396+
(void)SDL_EventState(SDL_TEXTINPUT, SDL_DISABLE);
4397+
(void)SDL_EventState(SDL_TEXTEDITING, SDL_DISABLE);
43984398
}
43994399

44004400
void

‎src/video/SDL_yuv.c

+4-3
Original file line numberDiff line numberDiff line change
@@ -601,9 +601,10 @@ SDL_ConvertPixels_ARGB8888_to_YUV(int width, int height, const void *src, int sr
601601
Uint8 *plane_interleaved_uv;
602602
Uint32 y_stride, uv_stride, y_skip, uv_skip;
603603

604-
GetYUVPlanes(width, height, dst_format, dst, dst_pitch,
605-
(const Uint8 **)&plane_y, (const Uint8 **)&plane_u, (const Uint8 **)&plane_v,
606-
&y_stride, &uv_stride);
604+
if (GetYUVPlanes(width, height, dst_format, dst, dst_pitch,
605+
(const Uint8 **)&plane_y, (const Uint8 **)&plane_u, (const Uint8 **)&plane_v,
606+
&y_stride, &uv_stride) != 0)
607+
return -1;
607608
plane_interleaved_uv = (plane_y + height * y_stride);
608609
y_skip = (y_stride - width);
609610

‎src/video/windows/SDL_windowskeyboard.c

+9-1
Original file line numberDiff line numberDiff line change
@@ -389,13 +389,21 @@ WIN_ShouldShowNativeUI()
389389
static void
390390
IME_Init(SDL_VideoData *videodata, HWND hwnd)
391391
{
392+
HRESULT hResult = S_OK;
393+
392394
if (videodata->ime_initialized)
393395
return;
394396

395397
videodata->ime_hwnd_main = hwnd;
396398
if (SUCCEEDED(WIN_CoInitialize())) {
397399
videodata->ime_com_initialized = SDL_TRUE;
398-
CoCreateInstance(&CLSID_TF_ThreadMgr, NULL, CLSCTX_INPROC_SERVER, &IID_ITfThreadMgr, (LPVOID *)&videodata->ime_threadmgr);
400+
hResult = CoCreateInstance(&CLSID_TF_ThreadMgr, NULL, CLSCTX_INPROC_SERVER, &IID_ITfThreadMgr, (LPVOID *)&videodata->ime_threadmgr);
401+
if (hResult != S_OK)
402+
{
403+
videodata->ime_available = SDL_FALSE;
404+
SDL_SetError("CoCreateInstance() failed, HRESULT is %08X", (unsigned int)hResult);
405+
return;
406+
}
399407
}
400408
videodata->ime_initialized = SDL_TRUE;
401409
videodata->ime_himm32 = SDL_LoadObject("imm32.dll");

‎src/video/windows/SDL_windowswindow.c

+8-4
Original file line numberDiff line numberDiff line change
@@ -739,15 +739,18 @@ WIN_GetWindowBordersSize(_THIS, SDL_Window * window, int *top, int *left, int *b
739739

740740
/* rcClient stores the size of the inner window, while rcWindow stores the outer size relative to the top-left
741741
* screen position; so the top/left values of rcClient are always {0,0} and bottom/right are {height,width} */
742-
GetClientRect(hwnd, &rcClient);
743-
GetWindowRect(hwnd, &rcWindow);
742+
if (!GetClientRect(hwnd, &rcClient))
743+
SDL_SetError("GetClientRect() failed, error %08X", (unsigned int)GetLastError());
744+
if (!GetWindowRect(hwnd, &rcWindow))
745+
SDL_SetError("GetWindowRect() failed, error %08X", (unsigned int)GetLastError());
744746

745747
/* convert the top/left values to make them relative to
746748
* the window; they will end up being slightly negative */
747749
ptDiff.y = rcWindow.top;
748750
ptDiff.x = rcWindow.left;
749751

750-
ScreenToClient(hwnd, &ptDiff);
752+
if (!ScreenToClient(hwnd, &ptDiff))
753+
SDL_SetError("ScreenToClient() failed, error %08X", (unsigned int)GetLastError());
751754

752755
rcWindow.top = ptDiff.y;
753756
rcWindow.left = ptDiff.x;
@@ -757,7 +760,8 @@ WIN_GetWindowBordersSize(_THIS, SDL_Window * window, int *top, int *left, int *b
757760
ptDiff.y = rcWindow.bottom;
758761
ptDiff.x = rcWindow.right;
759762

760-
ScreenToClient(hwnd, &ptDiff);
763+
if (!ScreenToClient(hwnd, &ptDiff))
764+
SDL_SetError("ScreenToClient() failed, error %08X", (unsigned int)GetLastError());
761765

762766
rcWindow.bottom = ptDiff.y;
763767
rcWindow.right = ptDiff.x;

0 commit comments

Comments
 (0)