Fixed bug 2330 - Debian bug report: SDL2 X11 driver buffer overflow with large X11 file descriptor

manuel.montezelo

Original bug report (note that it was against 2.0.0, it might have been fixed in between):  http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=733015

--------------------------------------------------------
Package: libsdl2-2.0-0
Version: 2.0.0+dfsg1-3
Severity: normal
Tags: patch

I have occasional crashes here caused by the X11 backend of SDL2. It seems to
be caused by the X11_Pending function trying to add a high number (> 1024)
file descriptor to a fd_set before doing a select on it to avoid busy waiting
on X11 events. This causes a buffer overflow because the file descriptor is
larger (or equal) than the limit FD_SETSIZE.

Attached is a possible workaround patch.

Please also keep in mind that fd_set are also used in following files which
may have similar problems.

src/audio/bsd/SDL_bsdaudio.c
src/audio/paudio/SDL_paudio.c
src/audio/qsa/SDL_qsa_audio.c
src/audio/sun/SDL_sunaudio.c
src/joystick/linux/SDL_sysjoystick.c


--------------------------------------------------------

On Tuesday 24 December 2013 00:43:13 Sven Eckelmann wrote:
> I have occasional crashes here caused by the X11 backend of SDL2. It seems
> to be caused by the X11_Pending function trying to add a high number (>
> 1024) file descriptor to a fd_set before doing a select on it to avoid busy
> waiting on X11 events. This causes a buffer overflow because the file
> descriptor is larger (or equal) than the limit FD_SETSIZE.


I personally experienced this problem while hacking on the python bindings
package for SDL2 [1] (while doing make runtest). But it easier to reproduce in
a smaller, synthetic testcase.

CMakeLists.txt

@@ -662,7 +662,7 @@ if(LIBC)
             _uitoa _ultoa strtol strtoul _i64toa _ui64toa strtoll strtoull
             atoi atof strcmp strncmp _stricmp strcasecmp _strnicmp strncasecmp
             vsscanf vsnprintf fopen64 fseeko fseeko64 sigaction setjmp
-            nanosleep sysconf sysctlbyname getauxval
+            nanosleep sysconf sysctlbyname getauxval poll
             )
       string(TOUPPER ${_FN} _UPPER)
       set(_HAVEVAR "HAVE_${_UPPER}")

configure

@@ -16631,7 +16631,7 @@ fi
 rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
 fi
 
-    for ac_func in malloc calloc realloc free getenv setenv putenv unsetenv qsort abs bcopy memset memcpy memmove wcslen wcslcpy wcslcat wcscmp strlen strlcpy strlcat strdup _strrev _strupr _strlwr strchr strrchr strstr itoa _ltoa _uitoa _ultoa strtol strtoul _i64toa _ui64toa strtoll strtoull atoi atof strcmp strncmp _stricmp strcasecmp _strnicmp strncasecmp vsscanf vsnprintf fopen64 fseeko fseeko64 sigaction setjmp nanosleep sysconf sysctlbyname getauxval
+    for ac_func in malloc calloc realloc free getenv setenv putenv unsetenv qsort abs bcopy memset memcpy memmove wcslen wcslcpy wcslcat wcscmp strlen strlcpy strlcat strdup _strrev _strupr _strlwr strchr strrchr strstr itoa _ltoa _uitoa _ultoa strtol strtoul _i64toa _ui64toa strtoll strtoull atoi atof strcmp strncmp _stricmp strcasecmp _strnicmp strncasecmp vsscanf vsnprintf fopen64 fseeko fseeko64 sigaction setjmp nanosleep sysconf sysctlbyname getauxval poll
 do :
   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
@@ -23771,6 +23771,8 @@ $as_echo "#define SDL_TIMER_UNIX 1" >>confdefs.h
         if test x$use_input_events = xyes; then
             SOURCES="$SOURCES $srcdir/src/core/linux/SDL_evdev*.c"
         fi
+        # Set up other core UNIX files
+        SOURCES="$SOURCES $srcdir/src/core/unix/*.c"
         ;;
     *-*-cygwin* | *-*-mingw32*)
         ARCH=win32

configure.in

@@ -268,7 +268,7 @@ if test x$enable_libc = xyes; then
         AC_DEFINE(HAVE_MPROTECT, 1, [ ])
         ]),
     )
-    AC_CHECK_FUNCS(malloc calloc realloc free getenv setenv putenv unsetenv qsort abs bcopy memset memcpy memmove wcslen wcslcpy wcslcat wcscmp strlen strlcpy strlcat strdup _strrev _strupr _strlwr strchr strrchr strstr itoa _ltoa _uitoa _ultoa strtol strtoul _i64toa _ui64toa strtoll strtoull atoi atof strcmp strncmp _stricmp strcasecmp _strnicmp strncasecmp vsscanf vsnprintf fopen64 fseeko fseeko64 sigaction setjmp nanosleep sysconf sysctlbyname getauxval)
+    AC_CHECK_FUNCS(malloc calloc realloc free getenv setenv putenv unsetenv qsort abs bcopy memset memcpy memmove wcslen wcslcpy wcslcat wcscmp strlen strlcpy strlcat strdup _strrev _strupr _strlwr strchr strrchr strstr itoa _ltoa _uitoa _ultoa strtol strtoul _i64toa _ui64toa strtoll strtoull atoi atof strcmp strncmp _stricmp strcasecmp _strnicmp strncasecmp vsscanf vsnprintf fopen64 fseeko fseeko64 sigaction setjmp nanosleep sysconf sysctlbyname getauxval poll)
 
     AC_CHECK_LIB(m, pow, [LIBS="$LIBS -lm"; EXTRA_LDFLAGS="$EXTRA_LDFLAGS -lm"])
     AC_CHECK_FUNCS(atan atan2 acos asin ceil copysign cos cosf fabs floor log pow scalbn sin sinf sqrt sqrtf tan tanf)
@@ -3352,6 +3352,8 @@ case "$host" in
         if test x$use_input_events = xyes; then
             SOURCES="$SOURCES $srcdir/src/core/linux/SDL_evdev*.c"
         fi       
+        # Set up other core UNIX files
+        SOURCES="$SOURCES $srcdir/src/core/unix/*.c"
         ;;
     *-*-cygwin* | *-*-mingw32*)
         ARCH=win32

include/SDL_config.h.cmake

@@ -181,6 +181,7 @@
 #cmakedefine HAVE_PTHREAD_SET_NAME_NP 1
 #cmakedefine HAVE_SEM_TIMEDWAIT 1
 #cmakedefine HAVE_GETAUXVAL 1
+#cmakedefine HAVE_POLL 1
 
 #elif __WIN32__
 #cmakedefine HAVE_STDARG_H 1

include/SDL_config.h.in

@@ -183,6 +183,7 @@
 #undef HAVE_PTHREAD_SET_NAME_NP
 #undef HAVE_SEM_TIMEDWAIT
 #undef HAVE_GETAUXVAL
+#undef HAVE_POLL
 
 #else
 #define HAVE_STDARG_H   1

src/audio/arts/SDL_artsaudio.h

@@ -42,7 +42,7 @@ struct SDL_PrivateAudioData
     Uint8 *mixbuf;
     int mixlen;
 
-    /* Support for audio timing using a timer, in addition to select() */
+    /* Support for audio timing using a timer, in addition to SDL_IOReady() */
     float frame_ticks;
     float next_frame;
 };

src/audio/netbsd/SDL_netbsdaudio.c

@@ -38,6 +38,7 @@
 
 #include "SDL_timer.h"
 #include "SDL_audio.h"
+#include "../../core/unix/SDL_poll.h"
 #include "../SDL_audio_c.h"
 #include "../SDL_audiodev_c.h"
 #include "SDL_netbsdaudio.h"
@@ -134,18 +135,11 @@ NETBSDAUDIO_WaitDevice(_THIS)
             SDL_Delay(ticks);
         }
     } else {
-        /* Use select() for audio synchronization */
+        /* Use SDL_IOReady() for audio synchronization */
-        fd_set fdset;
-        struct timeval timeout;
-
-        FD_ZERO(&fdset);
-        FD_SET(this->hidden->audio_fd, &fdset);
-        timeout.tv_sec = 10;
-        timeout.tv_usec = 0;
 #ifdef DEBUG_AUDIO
         fprintf(stderr, "Waiting for audio to get ready\n");
 #endif
-        if (select(this->hidden->audio_fd + 1, NULL, &fdset, NULL, &timeout)
+        if (SDL_IOReady(this->hidden->audio_fd, SDL_TRUE, 10 * 1000)
             <= 0) {
             const char *message =
                 "Audio timeout - buggy audio driver? (disabled)";

src/audio/netbsd/SDL_netbsdaudio.h

@@ -36,7 +36,7 @@ struct SDL_PrivateAudioData
     Uint8 *mixbuf;
     int mixlen;
 
-    /* Support for audio timing using a timer, in addition to select() */
+    /* Support for audio timing using a timer, in addition to SDL_IOReady() */
     float frame_ticks;
     float next_frame;
 };

src/audio/paudio/SDL_paudio.c

@@ -36,6 +36,7 @@
 #include "SDL_audio.h"
 #include "SDL_stdinc.h"
 #include "../SDL_audio_c.h"
+#include "../../core/unix/SDL_poll.h"
 #include "SDL_paudio.h"
 
 /* #define DEBUG_AUDIO */
@@ -137,44 +138,31 @@ PAUDIO_WaitDevice(_THIS)
             SDL_Delay(ticks);
         }
     } else {
+        int timeoutMS;
         audio_buffer paud_bufinfo;
 
-        /* Use select() for audio synchronization */
-        struct timeval timeout;
-        FD_ZERO(&fdset);
-        FD_SET(this->hidden->audio_fd, &fdset);
-
         if (ioctl(this->hidden->audio_fd, AUDIO_BUFFER, &paud_bufinfo) < 0) {
 #ifdef DEBUG_AUDIO
             fprintf(stderr, "Couldn't get audio buffer information\n");
 #endif
-            timeout.tv_sec = 10;
+            timeoutMS = 10 * 1000;
-            timeout.tv_usec = 0;
         } else {
-            long ms_in_buf = paud_bufinfo.write_buf_time;
+            timeoutMS = paud_bufinfo.write_buf_time;
-            timeout.tv_sec = ms_in_buf / 1000;
-            ms_in_buf = ms_in_buf - timeout.tv_sec * 1000;
-            timeout.tv_usec = ms_in_buf * 1000;
 #ifdef DEBUG_AUDIO
-            fprintf(stderr,
+            fprintf(stderr, "Waiting for write_buf_time=%d ms\n", timeoutMS);
-                    "Waiting for write_buf_time=%ld,%ld\n",
-                    timeout.tv_sec, timeout.tv_usec);
 #endif
         }
 
 #ifdef DEBUG_AUDIO
         fprintf(stderr, "Waiting for audio to get ready\n");
 #endif
-        if (select(this->hidden->audio_fd + 1, NULL, &fdset, NULL, &timeout)
+        if (SDL_IOReady(this->hidden->audio_fd, SDL_TRUE, timeoutMS) <= 0) {
-            <= 0) {
-            const char *message =
-                "Audio timeout - buggy audio driver? (disabled)";
             /*
              * In general we should never print to the screen,
              * but in this case we have no other way of letting
              * the user know what happened.
              */
-            fprintf(stderr, "SDL: %s - %s\n", strerror(errno), message);
+            fprintf(stderr, "SDL: %s - Audio timeout - buggy audio driver? (disabled)\n", strerror(errno));
             SDL_OpenedAudioDeviceDisconnected(this);
             /* Don't try to close - may hang */
             this->hidden->audio_fd = -1;
@@ -486,7 +474,7 @@ PAUDIO_OpenDevice(_THIS, void *handle, const char *devname, int iscapture)
         return SDL_SetError("Can't start audio play");
     }
 
-    /* Check to see if we need to use select() workaround */
+    /* Check to see if we need to use SDL_IOReady() workaround */
     if (workaround != NULL) {
         this->hidden->frame_ticks = (float) (this->spec.samples * 1000) /
             this->spec.freq;

src/audio/paudio/SDL_paudio.h

@@ -37,7 +37,7 @@ struct SDL_PrivateAudioData
     Uint8 *mixbuf;
     int mixlen;
 
-    /* Support for audio timing using a timer, in addition to select() */
+    /* Support for audio timing using a timer, in addition to SDL_IOReady() */
     float frame_ticks;
     float next_frame;
 };

src/audio/qsa/SDL_qsa_audio.c

@@ -45,6 +45,7 @@
 
 #include "SDL_timer.h"
 #include "SDL_audio.h"
+#include "../../core/unix/SDL_poll.h"
 #include "../SDL_audio_c.h"
 #include "SDL_qsa_audio.h"
 
@@ -113,67 +114,25 @@ QSA_InitAudioParams(snd_pcm_channel_params_t * cpars)
 static void
 QSA_WaitDevice(_THIS)
 {
-    fd_set wfds;
+    int result;
-    fd_set rfds;
-    int selectret;
-    struct timeval timeout;
 
-    if (!this->hidden->iscapture) {
-        FD_ZERO(&wfds);
-        FD_SET(this->hidden->audio_fd, &wfds);
-    } else {
-        FD_ZERO(&rfds);
-        FD_SET(this->hidden->audio_fd, &rfds);
-    }
-
-    do {
     /* Setup timeout for playing one fragment equal to 2 seconds          */
     /* If timeout occured than something wrong with hardware or driver    */
     /* For example, Vortex 8820 audio driver stucks on second DAC because */
     /* it doesn't exist !                                                 */
-        timeout.tv_sec = 2;
+    result = SDL_IOReady(this->hidden->audio_fd, !this->hidden->iscapture, 2 * 1000);
-        timeout.tv_usec = 0;
+    switch (result) {
-        this->hidden->timeout_on_wait = 0;
-
-        if (!this->hidden->iscapture) {
-            selectret =
-                select(this->hidden->audio_fd + 1, NULL, &wfds, NULL,
-                       &timeout);
-        } else {
-            selectret =
-                select(this->hidden->audio_fd + 1, &rfds, NULL, NULL,
-                       &timeout);
-        }
-
-        switch (selectret) {
     case -1:
-            {
+        SDL_SetError("QSA: SDL_IOReady() failed: %s", strerror(errno));
-                SDL_SetError("QSA: select() failed: %s", strerror(errno));
-                return;
-            }
         break;
     case 0:
-            {
         SDL_SetError("QSA: timeout on buffer waiting occured");
         this->hidden->timeout_on_wait = 1;
-                return;
-            }
         break;
     default:
-            {
+        this->hidden->timeout_on_wait = 0;
-                if (!this->hidden->iscapture) {
-                    if (FD_ISSET(this->hidden->audio_fd, &wfds)) {
-                        return;
-                    }
-                } else {
-                    if (FD_ISSET(this->hidden->audio_fd, &rfds)) {
-                        return;
-                    }
-                }
-            }
         break;
     }
-    } while (1);
 }
 
 static void

src/audio/sun/SDL_sunaudio.c

@@ -40,6 +40,7 @@
 
 #include "SDL_timer.h"
 #include "SDL_audio.h"
+#include "../../core/unix/SDL_poll.h"
 #include "../SDL_audio_c.h"
 #include "../SDL_audiodev_c.h"
 #include "SDL_sunaudio.h"
@@ -97,11 +98,7 @@ SUNAUDIO_WaitDevice(_THIS)
         }
     }
 #else
-    fd_set fdset;
+    SDL_IOReady(this->hidden->audio_fd, SDL_TRUE, -1);
-
-    FD_ZERO(&fdset);
-    FD_SET(this->hidden->audio_fd, &fdset);
-    select(this->hidden->audio_fd + 1, NULL, &fdset, NULL, NULL);
 #endif
 }
 

src/core/linux/SDL_ime.h

@@ -36,3 +36,5 @@ extern void SDL_IME_UpdateTextRect(SDL_Rect *rect);
 extern void SDL_IME_PumpEvents(void);
 
 #endif /* SDL_ime_h_ */
+
+/* vi: set ts=4 sw=4 expandtab: */

src/core/linux/SDL_udev.c

@@ -31,7 +31,10 @@
 
 #include <linux/input.h>
 
-#include "SDL.h"
+#include "SDL_assert.h"
+#include "SDL_loadso.h"
+#include "SDL_timer.h"
+#include "../unix/SDL_poll.h"
 
 static const char *SDL_UDEV_LIBS[] = {
 #ifdef SDL_UDEV_DYNAMIC
@@ -105,14 +108,7 @@ SDL_UDEV_hotplug_update_available(void)
 {
     if (_this->udev_mon != NULL) {
         const int fd = _this->udev_monitor_get_fd(_this->udev_mon);
-        fd_set fds;
+        if (SDL_IOReady(fd, SDL_FALSE, 0)) {
-        struct timeval tv;
-
-        FD_ZERO(&fds);
-        FD_SET(fd, &fds);
-        tv.tv_sec = 0;
-        tv.tv_usec = 0;
-        if ((select(fd+1, &fds, NULL, NULL, &tv) > 0) && (FD_ISSET(fd, &fds))) {
             return SDL_TRUE;
         }
     }

src/core/linux/SDL_udev.h

@@ -117,3 +117,5 @@ extern void SDL_UDEV_DelCallback(SDL_UDEV_Callback cb);
 #endif /* HAVE_LIBUDEV_H */
 
 #endif /* SDL_udev_h_ */
+
+/* vi: set ts=4 sw=4 expandtab: */

src/video/wayland/SDL_waylanddatamanager.c

@@ -30,6 +30,7 @@
 
 #include "SDL_stdinc.h"
 #include "SDL_assert.h"
+#include "../../core/unix/SDL_poll.h"
 
 #include "SDL_waylandvideo.h"
 #include "SDL_waylanddatamanager.h"
@@ -46,16 +47,8 @@ write_pipe(int fd, const void* buffer, size_t total_length, size_t *pos)
     sigset_t sig_set;
     sigset_t old_sig_set;
     struct timespec zerotime = {0};
-    fd_set set;
-    struct timeval timeout;
 
-    FD_ZERO(&set);
+    ready = SDL_IOReady(fd, SDL_TRUE, 1 * 1000);
-    FD_SET(fd, &set);
-
-    timeout.tv_sec = 1;
-    timeout.tv_usec = 0;
-
-    ready = select(fd + 1, NULL, &set, NULL, &timeout);
 
     sigemptyset(&sig_set);
     sigaddset(&sig_set, SIGPIPE);  
@@ -92,16 +85,7 @@ read_pipe(int fd, void** buffer, size_t* total_length, SDL_bool null_terminate)
     ssize_t bytes_read = 0;
     size_t pos = 0;
 
-    fd_set set;
+    ready = SDL_IOReady(fd, SDL_FALSE, 1 * 1000);
-    struct timeval timeout;
-
-    FD_ZERO(&set);
-    FD_SET(fd, &set);
-
-    timeout.tv_sec = 1;
-    timeout.tv_usec = 0;
-
-    ready = select(fd + 1, &set, NULL, NULL, &timeout);  
   
     if (ready == 0) {
         bytes_read = SDL_SetError("Pipe timeout");

src/video/wayland/SDL_waylandevents.c

@@ -27,6 +27,7 @@
 #include "SDL_assert.h"
 #include "SDL_log.h"
 
+#include "../../core/unix/SDL_poll.h"
 #include "../../events/SDL_sysevents.h"
 #include "../../events/SDL_events_c.h"
 #include "../../events/scancodes_xfree86.h"
@@ -74,16 +75,14 @@ void
 Wayland_PumpEvents(_THIS)
 {
     SDL_VideoData *d = _this->driverdata;
-    struct pollfd pfd[1];
 
-    pfd[0].fd = WAYLAND_wl_display_get_fd(d->display);
+    if (SDL_IOReady(WAYLAND_wl_display_get_fd(d->display), SDL_FALSE, 0)) {
-    pfd[0].events = POLLIN;
-    poll(pfd, 1, 0);
-
-    if (pfd[0].revents & POLLIN)
         WAYLAND_wl_display_dispatch(d->display);
+    }
     else
+    {
         WAYLAND_wl_display_dispatch_pending(d->display);
+    }
 }
 
 static void

src/video/x11/SDL_x11events.c

@@ -31,6 +31,7 @@
 #include "SDL_x11video.h"
 #include "SDL_x11touch.h"
 #include "SDL_x11xinput2.h"
+#include "../../core/unix/SDL_poll.h"
 #include "../../events/SDL_events_c.h"
 #include "../../events/SDL_mouse_c.h"
 #include "../../events/SDL_touch_c.h"
@@ -1409,18 +1410,9 @@ X11_Pending(Display * display)
     }
 
     /* More drastic measures are required -- see if X is ready to talk */
-    {
+    if (SDL_IOReady(ConnectionNumber(display), SDL_FALSE, 0)) {
-        static struct timeval zero_time;        /* static == 0 */
-        int x11_fd;
-        fd_set fdset;
-
-        x11_fd = ConnectionNumber(display);
-        FD_ZERO(&fdset);
-        FD_SET(x11_fd, &fdset);
-        if (select(x11_fd + 1, &fdset, NULL, NULL, &zero_time) == 1) {
         return (X11_XPending(display));
     }
-    }
 
     /* Oh well, nothing is ready .. */
     return (0);