Browse Source

log: Fix unlikely out-of-bounds issue.

In the unlikely case that the overflow check should fail,
the else clause would switch to the truncated stack message
without updating the len variable. This would contain the
return value from vsnprintf(), meaning it could point beyond
the buffer.

The subsequent code which trims NL and CR from the buffer,
would then read -- and possibly write -- out-of-bounds.

To fix this, we split the two joint conditions into separate
if-clauses, and adjust the len variable in the case where we
know the message buffer was truncated.
Eddy Jansson 2 weeks ago
parent
commit
2cb9a4fcc1
1 changed files with 14 additions and 8 deletions
  1. 14 8
      src/SDL_log.c

+ 14 - 8
src/SDL_log.c

@@ -612,15 +612,21 @@ void SDL_LogMessageV(int category, SDL_LogPriority priority, SDL_PRINTF_FORMAT_S
     }
     }
 
 
     // If message truncated, allocate and re-render
     // If message truncated, allocate and re-render
-    if (len >= sizeof(stack_buf) && SDL_size_add_check_overflow(len, 1, &len_plus_term)) {
-        // Allocate exactly what we need, including the zero-terminator
-        message = (char *)SDL_malloc(len_plus_term);
-        if (!message) {
-            return;
+    if (len >= sizeof(stack_buf)) {
+        if (SDL_size_add_check_overflow(len, 1, &len_plus_term)) {
+            // Allocate exactly what we need, including the zero-terminator
+            message = (char *)SDL_malloc(len_plus_term);
+            if (!message) {
+                return;
+            }
+            va_copy(aq, ap);
+            len = SDL_vsnprintf(message, len_plus_term, fmt, aq);
+            va_end(aq);
+        } else {
+            // Allocation would overflow, use truncated message
+            message = stack_buf;
+            len = sizeof(stack_buf);
         }
         }
-        va_copy(aq, ap);
-        len = SDL_vsnprintf(message, len_plus_term, fmt, aq);
-        va_end(aq);
     } else {
     } else {
         message = stack_buf;
         message = stack_buf;
     }
     }