Skip to content

Commit c09794c

Browse files
committed
libretro-common/file: NULL-check mallocs and unwind resources on OOM
Four OOM-deref sites across the file-I/O subsystem: nbio_linux.c (nbio_linux_open): handle = (struct nbio_linux_t*)malloc(sizeof(struct nbio_linux_t)); handle->fd = fd; /* NULL-deref on OOM */ handle->ctx = ctx; handle->len = lseek(fd, 0, SEEK_END); handle->ptr = malloc(handle->len); handle->busy = false; Two back-to-back mallocs with no NULL checks between the deref'ing field writes. Also leaks the open()'d fd and the io_setup()'d aio context on any OOM. The sibling nbio_stdio_open (same file family, line 121) already does this correctly with fclose-on-OOM and the nested malloc NULL-check; _linux_open was the lone holdout. nbio_unixmmap.c (nbio_mmap_unix_open): handle = malloc(sizeof(struct nbio_mmap_unix_t)); handle->fd = fd; /* NULL-deref on OOM */ handle->map_flags = map_flags[mode]; handle->len = _len; handle->ptr = ptr; Same unchecked-handle-malloc pattern; on OOM leaks the fd and the mmap'd region. nbio_windowsmmap.c (nbio_mmap_win32_open): handle = (struct nbio_mmap_win32_t*)malloc(sizeof(struct nbio_mmap_win32_t)); handle->file = file; /* NULL-deref on OOM */ handle->is_write = is_write; handle->len = len.QuadPart; handle->ptr = ptr; Same, Win32 variant; on OOM leaks the HANDLE and the MapViewOfFile mapping. archive_file_7z.c (sevenzip_file_read): *buf = malloc((size_t)(outsize + 1)); ((char*)(*buf))[outsize] = '\0'; /* NULL-deref on OOM */ memcpy(*buf, output + offset, outsize); Only one site in 7z extraction; on OOM NULL-derefs on the NUL-terminator write. Unlike the nbio sites, this one sits inside a for-loop that already has an error label; added res = SZ_ERROR_MEM / outsize = -1 / break so the existing '!(file_found && res == SZ_OK)' cleanup branch runs. Fixes in all four cases: NULL-check each malloc, unwind everything we already acquired above it, and return NULL / -1 as the respective function's error signal. free(NULL) / close(-1) are defined as safe no-ops where relevant but nothing here relies on that - each unwind is explicit about what it's releasing. Thread-safety: unchanged. nbio handles are owned by whichever thread opened them (typically a task queue thread); the 7z extractor runs on the main thread during content scans. Reachability: nbio_*_open is the entry point for every async file load in the engine - most relevant for menu thumbnails, where hundreds of handles can be in flight simultaneously on memory-constrained handhelds. 7z extraction runs whenever a 7z-packed core / content is loaded.
1 parent 8521da9 commit c09794c

4 files changed

Lines changed: 56 additions & 1 deletion

File tree

libretro-common/file/archive_file_7z.c

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,18 @@ static int64_t sevenzip_file_read(
269269
* We would however need to realloc anyways, because RetroArch
270270
* expects a \0 at the end, therefore we allocate new,
271271
* copy and free the old one. */
272-
*buf = malloc((size_t)(outsize + 1));
272+
/* NULL-check the malloc before the NUL-termination
273+
* write and the memcpy below dereference '*buf'. On
274+
* OOM mark the extraction failed and break out of the
275+
* file-search loop; the '!(file_found && res == SZ_OK)'
276+
* branch further down then forces outsize = -1 and
277+
* the teardown runs. */
278+
if (!(*buf = malloc((size_t)(outsize + 1))))
279+
{
280+
res = SZ_ERROR_MEM;
281+
outsize = -1;
282+
break;
283+
}
273284
((char*)(*buf))[outsize] = '\0';
274285
memcpy(*buf, output + offset, outsize);
275286
}

libretro-common/file/nbio/nbio_linux.c

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,10 +113,32 @@ static void *nbio_linux_open(const char * filename, unsigned mode)
113113
}
114114

115115
handle = (struct nbio_linux_t*)malloc(sizeof(struct nbio_linux_t));
116+
/* NULL-check the handle malloc: the next five lines dereferenced
117+
* 'handle' unconditionally, so OOM segfaulted. On failure we
118+
* also have to unwind the fd we open()'d above and the aio
119+
* context we io_setup()'d - both are resources that would be
120+
* owned by the handle had it been allocated. */
121+
if (!handle)
122+
{
123+
io_destroy(ctx);
124+
close(fd);
125+
return NULL;
126+
}
116127
handle->fd = fd;
117128
handle->ctx = ctx;
118129
handle->len = lseek(fd, 0, SEEK_END);
119130
handle->ptr = malloc(handle->len);
131+
/* Same pattern for the data buffer. Subsequent begin_read /
132+
* begin_write / iterate paths all assume handle->ptr is a valid
133+
* buffer of handle->len bytes; passing a NULL through them
134+
* would crash in the iocb setup. Unwind everything we got. */
135+
if (!handle->ptr)
136+
{
137+
free(handle);
138+
io_destroy(ctx);
139+
close(fd);
140+
return NULL;
141+
}
120142
handle->busy = false;
121143

122144
return handle;

libretro-common/file/nbio/nbio_unixmmap.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,17 @@ static void *nbio_mmap_unix_open(const char * filename, unsigned mode)
8484
}
8585

8686
handle = malloc(sizeof(struct nbio_mmap_unix_t));
87+
/* NULL-check the handle malloc: the four field writes below
88+
* would segfault on OOM. On failure munmap the region and
89+
* close the fd we acquired above - both were destined to be
90+
* owned by the handle and will leak otherwise. */
91+
if (!handle)
92+
{
93+
if (_len != 0)
94+
munmap(ptr, _len);
95+
close(fd);
96+
return NULL;
97+
}
8798
handle->fd = fd;
8899
handle->map_flags = map_flags[mode];
89100
handle->len = _len;

libretro-common/file/nbio/nbio_windowsmmap.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,17 @@ static void *nbio_mmap_win32_open(const char * filename, unsigned mode)
112112
CloseHandle(mem);
113113

114114
handle = (struct nbio_mmap_win32_t*)malloc(sizeof(struct nbio_mmap_win32_t));
115+
/* NULL-check the handle malloc: the field writes below would
116+
* segfault on OOM. On failure unmap the view and close the
117+
* file handle - both are resources destined to be owned by
118+
* the handle and will leak otherwise. */
119+
if (!handle)
120+
{
121+
if (ptr)
122+
UnmapViewOfFile(ptr);
123+
CloseHandle(file);
124+
return NULL;
125+
}
115126

116127
handle->file = file;
117128
handle->is_write = is_write;

0 commit comments

Comments
 (0)