Commit c0eada20198430109f5072d13b51cf4554a1dec3

Eric Curtin 2022-07-06T17:00:16

Fix assumption that DRI_DEVNAME begins at 0 (#5865) * Fix assumption that DRI_DEVNAME begins at 0 The existing logic of the code was to count every possible entry in KMSDRM_DRI_PATH. After this a for loop would start trying to open filename0, filename1, filename2, etc. In recent Linux kernels (say 5.18) with simpledrm, the lowest KMSDRM_DRI_DEVNAME is often /dev/dri/card1, rather than /dev/dri/card0, causing the code to fail once /dev/dri/card0 has failed to open. Running: modprobe foodrm && modprobe bardrm && rmmod foodrm before you try to run an application with SDL KMSDRM would have also made this fail. * Various changes from review - Removed newline and period from SDL error - Explicitely compare memcmp to zero (also changed to SDL_memcmp) - Changed memcpy to strncpy - Less aggressive line wrapping * Various changes from review - strncpy to SDL_strlcpy - removed size hardcodings for KMSDRM_DRI_PATHSIZE and KMSDRM_DRI_DEVNAMESIZE - made all KMSDRM_DRI defines, run-time variables to reduce bugs caused by these defines being more build-time on Linux and more run-rime on OpenBSD - renamed openbsd69orgreater variable to moderndri - altered comment from "if on OpenBSD" to add difference in 6.9 * Various changes from review - Use max size of destination, rather than max size of source - Less hardcodings

diff --git a/src/video/kmsdrm/SDL_kmsdrmvideo.c b/src/video/kmsdrm/SDL_kmsdrmvideo.c
index fd3a3cd..07a15ae 100644
--- a/src/video/kmsdrm/SDL_kmsdrmvideo.c
+++ b/src/video/kmsdrm/SDL_kmsdrmvideo.c
@@ -53,131 +53,112 @@
 #include <errno.h>
 
 #ifdef __OpenBSD__
-static SDL_bool openbsd69orgreater = SDL_FALSE;
-#define KMSDRM_DRI_PATH openbsd69orgreater ? "/dev/dri/" : "/dev/"
-#define KMSDRM_DRI_DEVFMT openbsd69orgreater ? "%scard%d" : "%sdrm%d"
-#define KMSDRM_DRI_DEVNAME openbsd69orgreater ? "card" : "drm"
-#define KMSDRM_DRI_DEVNAMESIZE openbsd69orgreater ? 4 : 3
-#define KMSDRM_DRI_CARDPATHFMT openbsd69orgreater ? "/dev/dri/card%d" : "/dev/drm%d"
+static SDL_bool moderndri = SDL_FALSE;
 #else
-#define KMSDRM_DRI_PATH "/dev/dri/"
-#define KMSDRM_DRI_DEVFMT "%scard%d"
-#define KMSDRM_DRI_DEVNAME "card"
-#define KMSDRM_DRI_DEVNAMESIZE 4
-#define KMSDRM_DRI_CARDPATHFMT "/dev/dri/card%d"
+static SDL_bool moderndri = SDL_TRUE;
 #endif
 
+static char kmsdrm_dri_path[16];
+static int kmsdrm_dri_pathsize = 0;
+static char kmsdrm_dri_devname[8];
+static int kmsdrm_dri_devnamesize = 0;
+static char kmsdrm_dri_cardpath[32];
+
 #ifndef EGL_PLATFORM_GBM_MESA
 #define EGL_PLATFORM_GBM_MESA 0x31D7
 #endif
 
 static int
-check_modestting(int devindex)
+get_driindex(void)
 {
-    SDL_bool available = SDL_FALSE;
-    char device[512];
+    int available = -ENOENT;
+    char device[sizeof(kmsdrm_dri_cardpath)];
     int drm_fd;
     int i;
+    int devindex = -1;
+    DIR *folder;
 
-    SDL_snprintf(device, sizeof (device), KMSDRM_DRI_DEVFMT, KMSDRM_DRI_PATH, devindex);
-
-    drm_fd = open(device, O_RDWR | O_CLOEXEC);
-    if (drm_fd >= 0) {
-        if (SDL_KMSDRM_LoadSymbols()) {
-            drmModeRes *resources = KMSDRM_drmModeGetResources(drm_fd);
-            if (resources) {
-                SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO,
-                  KMSDRM_DRI_DEVFMT
-                  " connector, encoder and CRTC counts are: %d %d %d",
-                  KMSDRM_DRI_PATH, devindex,
-                  resources->count_connectors, resources->count_encoders,
-                  resources->count_crtcs);
-
-                if (resources->count_connectors > 0
-                 && resources->count_encoders > 0
-                 && resources->count_crtcs > 0)
-                {
-                    available = SDL_FALSE;
-                    for (i = 0; i < resources->count_connectors; i++) {
-                        drmModeConnector *conn = KMSDRM_drmModeGetConnector(drm_fd,
-                            resources->connectors[i]);
-
-                        if (!conn) {
-                            continue;
-                        }
-
-                        if (conn->connection == DRM_MODE_CONNECTED && conn->count_modes) {
-                            if (SDL_GetHintBoolean(SDL_HINT_KMSDRM_REQUIRE_DRM_MASTER, SDL_TRUE)) {
-                                /* Skip this device if we can't obtain DRM master */
-                                KMSDRM_drmSetMaster(drm_fd);
-                                if (KMSDRM_drmAuthMagic(drm_fd, 0) == -EACCES) {
+    SDL_strlcpy(device, kmsdrm_dri_path, sizeof(device));
+    folder = opendir(device);
+    if (!folder) {
+        SDL_SetError("Failed to open directory '%s'", device);
+        return -ENOENT;
+    }
+
+    SDL_strlcpy(device + kmsdrm_dri_pathsize, kmsdrm_dri_devname,
+                sizeof(device) - kmsdrm_dri_devnamesize);
+    for (struct dirent *res; (res = readdir(folder));) {
+        if (SDL_memcmp(res->d_name, kmsdrm_dri_devname,
+                       kmsdrm_dri_devnamesize) == 0) {
+            SDL_strlcpy(device + kmsdrm_dri_pathsize + kmsdrm_dri_devnamesize,
+                        res->d_name + kmsdrm_dri_devnamesize,
+                        sizeof(device) - kmsdrm_dri_pathsize -
+                            kmsdrm_dri_devnamesize);
+
+            drm_fd = open(device, O_RDWR | O_CLOEXEC);
+            if (drm_fd >= 0) {
+                devindex = SDL_atoi(device + kmsdrm_dri_pathsize +
+                                    kmsdrm_dri_devnamesize);
+                if (SDL_KMSDRM_LoadSymbols()) {
+                    drmModeRes *resources = KMSDRM_drmModeGetResources(drm_fd);
+                    if (resources) {
+                        SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO,
+                                     "%s%d connector, encoder and CRTC counts are: %d %d %d",
+                                     kmsdrm_dri_cardpath, devindex,
+                                     resources->count_connectors,
+                                     resources->count_encoders,
+                                     resources->count_crtcs);
+
+                        if (resources->count_connectors > 0 &&
+                            resources->count_encoders > 0 &&
+                            resources->count_crtcs > 0) {
+                            available = -ENOENT;
+                            for (i = 0; i < resources->count_connectors; i++) {
+                                drmModeConnector *conn =
+                                    KMSDRM_drmModeGetConnector(
+                                        drm_fd, resources->connectors[i]);
+
+                                if (!conn) {
                                     continue;
                                 }
-                            }
 
-                            available = SDL_TRUE;
-                            break;
-                        }
+                                if (conn->connection == DRM_MODE_CONNECTED &&
+                                    conn->count_modes) {
+                                    if (SDL_GetHintBoolean(
+                                            SDL_HINT_KMSDRM_REQUIRE_DRM_MASTER,
+                                            SDL_TRUE)) {
+                                        /* Skip this device if we can't obtain
+                                         * DRM master */
+                                        KMSDRM_drmSetMaster(drm_fd);
+                                        if (KMSDRM_drmAuthMagic(drm_fd, 0) ==
+                                            -EACCES) {
+                                            continue;
+                                        }
+                                    }
+
+                                    available = devindex;
+                                    break;
+                                }
 
-                        KMSDRM_drmModeFreeConnector(conn);
+                                KMSDRM_drmModeFreeConnector(conn);
+                            }
+                        }
+                        KMSDRM_drmModeFreeResources(resources);
                     }
+                    SDL_KMSDRM_UnloadSymbols();
                 }
-                KMSDRM_drmModeFreeResources(resources);
+                close(drm_fd);
             }
-            SDL_KMSDRM_UnloadSymbols();
-        }
-        close(drm_fd);
-    }
-
-    return available;
-}
-
-static int get_dricount(void)
-{
-    int devcount = 0;
-    struct dirent *res;
-    struct stat sb;
-    DIR *folder;
-
-    if (!(stat(KMSDRM_DRI_PATH, &sb) == 0
-                && S_ISDIR(sb.st_mode))) {
-        /*printf("The path %s cannot be opened or is not available\n", KMSDRM_DRI_PATH);*/
-        return 0;
-    }
 
-    if (access(KMSDRM_DRI_PATH, F_OK) == -1) {
-        /*printf("The path %s cannot be opened\n", KMSDRM_DRI_PATH);*/
-        return 0;
-    }
-
-    folder = opendir(KMSDRM_DRI_PATH);
-    if (folder) {
-        while ((res = readdir(folder))) {
-            int len = SDL_strlen(res->d_name);
-            if (len > KMSDRM_DRI_DEVNAMESIZE && SDL_strncmp(res->d_name,
-                      KMSDRM_DRI_DEVNAME, KMSDRM_DRI_DEVNAMESIZE) == 0) {
-                devcount++;
-            }
+            SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO,
+                         "Failed to open KMSDRM device %s, errno: %d\n", device,
+                         errno);
         }
-        closedir(folder);
     }
 
-    return devcount;
-}
-
-static int
-get_driindex(void)
-{
-    const int devcount = get_dricount();
-    int i;
-
-    for (i = 0; i < devcount; i++) {
-        if (check_modestting(i)) {
-            return i;
-        }
-    }
+    closedir(folder);
 
-    return -ENOENT;
+    return available;
 }
 
 static int
@@ -193,10 +174,24 @@ KMSDRM_Available(void)
     if (!(uname(&nameofsystem) < 0)) {
         releaseversion = SDL_atof(nameofsystem.release);
         if (releaseversion >= 6.9) {
-            openbsd69orgreater = SDL_TRUE;
+            moderndri = SDL_TRUE;
         }
     }
 #endif
+
+    if (moderndri) {
+        SDL_strlcpy(kmsdrm_dri_path, "/dev/dri/", sizeof(kmsdrm_dri_path));
+        SDL_strlcpy(kmsdrm_dri_devname, "card", sizeof(kmsdrm_dri_devname));
+    } else {
+        SDL_strlcpy(kmsdrm_dri_path, "/dev/", sizeof(kmsdrm_dri_path));
+        SDL_strlcpy(kmsdrm_dri_devname, "drm", sizeof(kmsdrm_dri_devname));
+    }
+
+    kmsdrm_dri_pathsize = SDL_strlen(kmsdrm_dri_path);
+    kmsdrm_dri_devnamesize = SDL_strlen(kmsdrm_dri_devname);
+    SDL_snprintf(kmsdrm_dri_cardpath, sizeof(kmsdrm_dri_cardpath), "%s%s",
+                 kmsdrm_dri_path, kmsdrm_dri_devname);
+
     ret = get_driindex();
     if (ret >= 0)
         return 1;
@@ -732,8 +727,9 @@ KMSDRM_InitDisplays (_THIS) {
     int ret = 0;
     int i;
 
-    /* Open /dev/dri/cardNN (/dev/drmN if on OpenBSD) */
-    SDL_snprintf(viddata->devpath, sizeof(viddata->devpath), KMSDRM_DRI_CARDPATHFMT, viddata->devindex);
+    /* Open /dev/dri/cardNN (/dev/drmN if on OpenBSD version less than 6.9) */
+    SDL_snprintf(viddata->devpath, sizeof(viddata->devpath), "%s%d",
+                 kmsdrm_dri_cardpath, viddata->devindex);
 
     SDL_LogDebug(SDL_LOG_CATEGORY_VIDEO, "Opening device %s", viddata->devpath);
     viddata->drm_fd = open(viddata->devpath, O_RDWR | O_CLOEXEC);