Skip to content
47 changes: 44 additions & 3 deletions src_c/_sdl2/controller.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,19 @@ static PyObject *
controller_module_init(PyObject *module, PyObject *_null)
{
if (!SDL_WasInit(SDL_INIT_GAMECONTROLLER)) {
#if SDL_VERSION_ATLEAST(3, 0, 0)
if (!SDL_InitSubSystem(SDL_INIT_GAMECONTROLLER)) {
#else
if (SDL_InitSubSystem(SDL_INIT_GAMECONTROLLER)) {
#endif
return RAISE(pgExc_SDLError, SDL_GetError());
}
}
#if SDL_VERSION_ATLEAST(3, 0, 0)
SDL_SetGamepadEventsEnabled(SDL_TRUE);
#else
SDL_GameControllerEventState(SDL_ENABLE);
#endif

Py_RETURN_NONE;
}
Expand Down Expand Up @@ -87,7 +95,16 @@ controller_module_get_count(PyObject *module, PyObject *_null)
{
CONTROLLER_INIT_CHECK();

#if SDL_VERSION_ATLEAST(3, 0, 0)
int count;
SDL_JoystickID *joysticks = SDL_GetJoysticks(&count);
if (!joysticks) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
SDL_free(joysticks);
#else
int count = SDL_NumJoysticks();
#endif
if (count < 0) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
Expand Down Expand Up @@ -290,7 +307,11 @@ controller_set_mapping(pgControllerObject *self, PyObject *args,

char guid_str[64];
SDL_Joystick *joy = SDL_GameControllerGetJoystick(self->controller);
#if SDL_VERSION_ATLEAST(3, 0, 0)
SDL_GUIDToString(SDL_JoystickGetGUID(joy), guid_str, 63);
#else
SDL_JoystickGetGUIDString(SDL_JoystickGetGUID(joy), guid_str, 63);
#endif

PyObject *key, *value;
const char *key_str, *value_str;
Expand Down Expand Up @@ -371,10 +392,17 @@ controller_rumble(pgControllerObject *self, PyObject *args, PyObject *kwargs)
low_freq = MAX(MIN(low_freq, 1.0f), 0.0f) * 65535;
high_freq = MAX(MIN(high_freq, 1.0f), 0.0f) * 65535;

#if SDL_VERSION_ATLEAST(3, 0, 0)
SDL_bool success = SDL_GameControllerRumble(
self->controller, (Uint16)low_freq, (Uint16)high_freq, duration);

return PyBool_FromLong(success == SDL_TRUE);
#else
int success = SDL_GameControllerRumble(self->controller, (Uint16)low_freq,
(Uint16)high_freq, duration);

return PyBool_FromLong(success == 0);

#endif
}

static PyObject *
Expand All @@ -384,7 +412,9 @@ controller_stop_rumble(pgControllerObject *self, PyObject *_null)
if (!self->controller) {
return RAISE(pgExc_SDLError, "Controller is not initialized");
}
SDL_GameControllerRumble(self->controller, 0, 0, 1);
if (SDL_GameControllerRumble(self->controller, 0, 0, 1) == SDL_FALSE) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this breaks the SDL2 case. Why not leave this code as it is?

Py_RETURN_NONE;
}

Expand Down Expand Up @@ -437,7 +467,18 @@ controller_new(PyTypeObject *subtype, PyObject *args, PyObject *kwargs)

CONTROLLER_INIT_CHECK();

if (id >= SDL_NumJoysticks() || !SDL_IsGameController(id)) {
#if SDL_VERSION_ATLEAST(3, 0, 0)
int joycount;
SDL_JoystickID *joysticks = SDL_GetJoysticks(&joycount);
if (!joysticks) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
SDL_free(joysticks);
#else
int joycount = SDL_NumJoysticks();
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given this is used in 2 places, does it make more sense to define this as a helper function?


if (id >= joycount || !SDL_IsGameController(id)) {
return RAISE(pgExc_SDLError, "Invalid index");
}

Expand Down
6 changes: 5 additions & 1 deletion src_c/_sdl2/meson.build
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
cython_base = '../cython/pygame/_sdl2'

if sdl_api != 3
_sdl2_audio = py.extension_module(
'audio',
fs.is_file('audio.c') ? 'audio.c' : cython_base / 'audio.pyx',
Expand All @@ -8,7 +9,9 @@ _sdl2_audio = py.extension_module(
install: true,
subdir: pg / '_sdl2',
)
endif

if sdl_api != 3
_sdl2_video = py.extension_module(
'video',
fs.is_file('video.c') ? 'video.c' : cython_base / 'video.pyx',
Expand Down Expand Up @@ -37,6 +40,7 @@ if sdl_mixer_dep.found()
subdir: pg / '_sdl2',
)
endif
endif

_sdl2_sdl2 = py.extension_module(
'sdl2',
Expand All @@ -47,7 +51,7 @@ _sdl2_sdl2 = py.extension_module(
subdir: pg / '_sdl2',
)

# This is not a cython file
# These are not cython files
_sdl2_touch = py.extension_module(
'touch',
'touch.c',
Expand Down
75 changes: 64 additions & 11 deletions src_c/_sdl2/touch.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,20 +25,48 @@
static PyObject *
pg_touch_num_devices(PyObject *self, PyObject *_null)
{
return PyLong_FromLong(SDL_GetNumTouchDevices());
int count;
#if SDL_VERSION_ATLEAST(3, 0, 0)

SDL_TouchID *devices = SDL_GetTouchDevices(&count);
if (devices == NULL) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
SDL_free(devices);
#else
count = SDL_GetNumTouchDevices();
#endif
return PyLong_FromLong(count);
}

static PyObject *
pg_touch_get_device(PyObject *self, PyObject *index)
pg_touch_get_device(PyObject *self, PyObject *index_obj)
{
SDL_TouchID touchid;
if (!PyLong_Check(index)) {
if (!PyLong_Check(index_obj)) {
return RAISE(PyExc_TypeError,
"index must be an integer "
"specifying a device to get the ID for");
}

touchid = SDL_GetTouchDevice(PyLong_AsLong(index));
int index = PyLong_AsLong(index_obj);
if (PyErr_Occurred()) {
return NULL; // exception already set
}
#if SDL_VERSION_ATLEAST(3, 0, 0)
int count;
SDL_TouchID *devices = SDL_GetTouchDevices(&count);
if (devices == NULL) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
if (index < 0 || index >= count) {
SDL_free(devices);
return RAISE(PyExc_IndexError, "index is out of bounds");
}
touchid = devices[index];
SDL_free(devices);
#else
touchid = SDL_GetTouchDevice(index);
#endif
if (touchid == 0) {
/* invalid index */
return RAISE(pgExc_SDLError, SDL_GetError());
Expand All @@ -47,21 +75,32 @@ pg_touch_get_device(PyObject *self, PyObject *index)
}

static PyObject *
pg_touch_num_fingers(PyObject *self, PyObject *device_id)
pg_touch_num_fingers(PyObject *self, PyObject *device_id_obj)
{
int fingercount;
if (!PyLong_Check(device_id)) {
if (!PyLong_Check(device_id_obj)) {
return RAISE(PyExc_TypeError,
"device_id must be an integer "
"specifying a touch device");
}
int device_id = PyLong_AsLongLong(device_id_obj);
if (PyErr_Occurred()) {
return NULL; // exception already set
}

VIDEO_INIT_CHECK();

fingercount = SDL_GetNumTouchFingers(PyLong_AsLongLong(device_id));
#if SDL_VERSION_ATLEAST(3, 0, 0)
SDL_Finger **fingers = SDL_GetTouchFingers(device_id, &fingercount);
if (fingers == NULL) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
SDL_free(fingers);
#else
fingercount = SDL_GetNumTouchFingers(device_id);
if (fingercount == 0) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
#endif
Comment on lines +99 to +103
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Do not treat “0 fingers” as an error in the SDL2 path.

SDL_GetNumTouchFingers returns a negative value on error; 0 is a valid “no active fingers” state. Current code raises SDLError when fingercount == 0, which is incorrect and will spuriously error on valid states. Fix by checking for < 0 instead of == 0. See SDL2 docs.

Apply this diff:

 #else
-    fingercount = SDL_GetNumTouchFingers(device_id);
-    if (fingercount == 0) {
-        return RAISE(pgExc_SDLError, SDL_GetError());
-    }
+    fingercount = SDL_GetNumTouchFingers(device_id);
+    if (fingercount < 0) {
+        return RAISE(pgExc_SDLError, SDL_GetError());
+    }
 #endif
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fingercount = SDL_GetNumTouchFingers(device_id);
if (fingercount == 0) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
#endif
#else
fingercount = SDL_GetNumTouchFingers(device_id);
- if (fingercount == 0) {
- return RAISE(pgExc_SDLError, SDL_GetError());
if (fingercount < 0) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
#endif
🤖 Prompt for AI Agents
In src_c/_sdl2/touch.c around lines 99 to 103, the code incorrectly treats a
fingercount of 0 as an SDL error; change the check to treat only negative return
values as errors by testing for fingercount < 0 and calling
RAISE(pgExc_SDLError, SDL_GetError()) only in that case, allowing fingercount ==
0 to be treated as a valid "no active fingers" state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fake news

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, meaning the current code in the diff is actually correct in treating fingercount == 0 as an error condition.

My previous suggestion was wrong - the code should indeed raise an SDL error when SDL_GetNumTouchFingers returns 0, as this indicates a failure condition, not a valid "no active fingers" state. Thank you for the correction.


✏️ Learnings added
Learnt from: ankith26
PR: pygame-community/pygame-ce#3427
File: src_c/_sdl2/touch.c:99-103
Timestamp: 2025-08-31T06:44:48.360Z
Learning: SDL_GetNumTouchFingers() returns 0 on failure, not as a valid "no fingers" state. A return value of 0 should be treated as an error condition and SDL_GetError() should be called to get the error message.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

return PyLong_FromLong(fingercount);
}
#if !defined(BUILD_STATIC)
Expand Down Expand Up @@ -95,14 +134,28 @@ pg_touch_get_finger(PyObject *self, PyObject *args, PyObject *kwargs)
}

VIDEO_INIT_CHECK();

#if SDL_VERSION_ATLEAST(3, 0, 0)
int fingercount;
SDL_Finger **fingers = SDL_GetTouchFingers(touchid, &fingercount);
if (fingers == NULL) {
return RAISE(pgExc_SDLError, SDL_GetError());
}
if (index < 0 || index >= fingercount) {
SDL_free(fingers);
return RAISE(PyExc_IndexError, "index is out of bounds");
}
finger = fingers[index];
SDL_free(fingers);
#else
Comment on lines 136 to +149
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix use-after-free in pg_touch_get_finger (SDL3 path).

SDL_GetTouchFingers returns a single allocation (array + finger data) that must be freed with SDL_free; finger pointers become invalid after free. The code currently assigns finger = fingers[index], calls SDL_free(fingers), and then dereferences finger later to build the dict — this is a UAF bug. Copy the fields you need before freeing, and then build the Python dict from those copies. See SDL3 docs: returned array is a single allocation to free once.

Apply this diff:

 #if SDL_VERSION_ATLEAST(3, 0, 0)
     int fingercount;
     SDL_Finger **fingers = SDL_GetTouchFingers(touchid, &fingercount);
     if (fingers == NULL) {
         return RAISE(pgExc_SDLError, SDL_GetError());
     }
     if (index < 0 || index >= fingercount) {
         SDL_free(fingers);
         return RAISE(PyExc_IndexError, "index is out of bounds");
     }
-    finger = fingers[index];
-    SDL_free(fingers);
+    finger = fingers[index];
+    SDL_FingerID fid = finger->id;
+    float fx = finger->x;
+    float fy = finger->y;
+    float fpressure = finger->pressure;
+    SDL_free(fingers);
 #else
-    if (!(finger = SDL_GetTouchFinger(touchid, index))) {
+    if (!(finger = SDL_GetTouchFinger(touchid, index))) {
         Py_RETURN_NONE;
     }
+    SDL_FingerID fid = finger->id;
+    float fx = finger->x;
+    float fy = finger->y;
+    float fpressure = finger->pressure;
 #endif
@@
-    _pg_insobj(fingerobj, "id", PyLong_FromLongLong(finger->id));
-    _pg_insobj(fingerobj, "x", PyFloat_FromDouble(finger->x));
-    _pg_insobj(fingerobj, "y", PyFloat_FromDouble(finger->y));
-    _pg_insobj(fingerobj, "pressure", PyFloat_FromDouble(finger->pressure));
+    _pg_insobj(fingerobj, "id", PyLong_FromLongLong(fid));
+    _pg_insobj(fingerobj, "x", PyFloat_FromDouble(fx));
+    _pg_insobj(fingerobj, "y", PyFloat_FromDouble(fy));
+    _pg_insobj(fingerobj, "pressure", PyFloat_FromDouble(fpressure));

Also applies to: 160-163

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think so

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

The user ankith26 has disagreed with my assessment, and given their retrieved learning about SDL internals, they likely have a better understanding. Let me acknowledge this and step back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hang on the bot is correct actually, just checked the source. This is an actual issue, though the way I would fix it is by freeing fingers at the end of the function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

if (!(finger = SDL_GetTouchFinger(touchid, index))) {
Py_RETURN_NONE;
}
#endif

fingerobj = PyDict_New();
if (!fingerobj)
if (!fingerobj) {
return NULL;
}

_pg_insobj(fingerobj, "id", PyLong_FromLongLong(finger->id));
_pg_insobj(fingerobj, "x", PyFloat_FromDouble(finger->x));
Expand Down
9 changes: 8 additions & 1 deletion src_c/cython/pygame/_sdl2/audio.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,14 @@

from .sdl2 cimport *

cdef extern from "SDL.h" nogil:
cdef extern from * nogil:
"""
#ifdef PG_SDL3
#include <SDL3/SDL.h>
#else
#include <SDL.h>
#endif
"""
# https://wiki.libsdl.org/SDL_OpenAudioDevice
# https://wiki.libsdl.org/SDL_CloseAudioDevice
# https://wiki.libsdl.org/SDL_AudioSpec
Expand Down
12 changes: 11 additions & 1 deletion src_c/cython/pygame/_sdl2/sdl2.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,17 @@
from libc.string cimport memset
from libc.stdio cimport *

cdef extern from "SDL.h" nogil:
cdef extern from * nogil:
"""
#ifdef PG_SDL3
#include <SDL3/SDL.h>
#define SDL_INIT_TIMER 0
#define SDL_INIT_EVERYTHING 0
#define SDL_INIT_NOPARACHUTE 0
#else
#include <SDL.h>
#endif
"""
# SDL_stdinc.h provides the real ones based on platform.
ctypedef char Sint8
ctypedef unsigned char Uint8
Expand Down
3 changes: 0 additions & 3 deletions src_c/meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,7 @@ gfxdraw = py.extension_module(
endif

# pygame._sdl2
# TODO: support SDL3
if sdl_api != 3
subdir('_sdl2')
endif

# pygame._camera
pg_camera_sources = ['_camera.c']
Expand Down
9 changes: 7 additions & 2 deletions src_py/_sdl2/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
from pygame.version import SDL

if __import__("sys").platform not in ("wasi", "emscripten"):
from .audio import * # pylint: disable=wildcard-import; lgtm[py/polluting-import]
if SDL < (3, 0, 0):
from .audio import * # pylint: disable=wildcard-import; lgtm[py/polluting-import]
from .sdl2 import * # pylint: disable=wildcard-import; lgtm[py/polluting-import]
from .video import * # pylint: disable=wildcard-import; lgtm[py/polluting-import]

if SDL < (3, 0, 0):
from .video import * # pylint: disable=wildcard-import; lgtm[py/polluting-import]
Loading