mirror of
https://github.com/libretro/RetroArch.git
synced 2026-11-07 08:36:02 -10:00
test/libretro-db: build samples under AddressSanitizer in CI
The previous commit added a libretrodb_leak_test that, under
ASan, catches reintroduced leaks in libretrodb_open / close /
cursor_close. The default Linux-libretro-db-samples.yml
workflow build is plain `make clean all` though, so a future
regression would not actually be caught in CI -- the test would
build and run, exit 0, and the leak would slip through.
Make ASan + UBSan the default for this workflow. Three changes:
1. .github/workflows/Linux-libretro-db-samples.yml: add a
MAKE_ARGS env var = "SANITIZER=address,undefined" and pass
it to the per-sample 'make clean all' invocation.
2. libretro-db/samples/rmsgpack/Makefile and
libretro-db/samples/libretrodb/Makefile (libretrodb's was
added in the previous commit): add the conventional
SANITIZER opt-in block:
ifneq ($(SANITIZER),)
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
endif
Matches the convention used in samples/tasks/http/Makefile.
Default build is unchanged; the workflow opts in via
MAKE_ARGS.
3. libretro-db/samples/rmsgpack/rmsgpack_overflow_test.c:
close a pre-existing 7-allocation / 336-byte leak in the
test harness itself. The test called intfstream_close on
each open_memory'd stream but never freed the
intfstream_t struct, mirroring the same convention-
mismatch as the libretrodb_open path the previous commit
fixed in production. Without this, ASan would report
this test as failing under the new SANITIZER=address
build.
Local simulation of the workflow under ASan:
- rmsgpack_overflow_test: 7 SUCCESS lines, exit 0, no ASan
output.
- libretrodb_leak_test: 4 SUCCESS lines, exit 0, no ASan
output.
Reverting the production fix in libretrodb.c (the previous
commit) makes libretrodb_leak_test report 460 bytes leaked
across 10 allocations under this workflow, which is what we
want.
This commit is contained in:
1 parent
7d6477b1aa
commit
c82db9f7db
6 files changed
+342
-5
No files matched your search
@@ -33,6 +33,14 @@ jobs:
|
||||
- name: Build and run samples
|
||||
shell: bash
|
||||
working-directory: libretro-db/samples
|
||||
env:
|
||||
# Build samples under AddressSanitizer + UndefinedBehaviorSanitizer.
|
||||
# Both libretro-db sample tests support this via the SANITIZER=
|
||||
# convention also used in samples/tasks/http/Makefile. ASan
|
||||
# catches any reintroduced leak (intfstream_t in libretrodb_open
|
||||
# / cursor_close, etc.) and any out-of-bounds memory access, so
|
||||
# this is the regression-protective build for these tests.
|
||||
MAKE_ARGS: "SANITIZER=address,undefined"
|
||||
run: |
|
||||
set -u
|
||||
set -o pipefail
|
||||
@@ -42,6 +50,7 @@ jobs:
|
||||
# failure. These are built AND executed.
|
||||
declare -a RUN_TARGETS=(
|
||||
rmsgpack_overflow_test
|
||||
libretrodb_leak_test
|
||||
)
|
||||
|
||||
# Per-binary run command (overrides ./<binary> if present).
|
||||
@@ -86,7 +95,7 @@ jobs:
|
||||
fi
|
||||
|
||||
# Build
|
||||
if ! ( cd "$d" && make clean all ); then
|
||||
if ! ( cd "$d" && make clean all $MAKE_ARGS ); then
|
||||
printf '\n::error title=Build failed::%s failed to build\n' "$rel"
|
||||
fails=$((fails+1))
|
||||
continue
|
||||
|
||||
@@ -184,8 +184,16 @@ clean:
|
||||
|
||||
void libretrodb_close(libretrodb_t *db)
|
||||
{
|
||||
/* intfstream_close closes the inner file but does not free
|
||||
* the intfstream_t struct itself (existing libretro-common
|
||||
* convention). Match the cleanup pattern used by
|
||||
* core_info.c / core_backup.c / cdfs.c / rpng_encode.c which
|
||||
* also free the struct after closing. */
|
||||
if (db->fd)
|
||||
{
|
||||
intfstream_close(db->fd);
|
||||
free(db->fd);
|
||||
}
|
||||
if (db->path && *db->path)
|
||||
free(db->path);
|
||||
db->path = NULL;
|
||||
@@ -246,8 +254,28 @@ int libretrodb_open(const char *path, libretrodb_t *db, bool write)
|
||||
return 0;
|
||||
|
||||
error:
|
||||
/* Free the strdup'd path (assigned at line 209) on the error
|
||||
* path: pre-this-commit it was leaked unconditionally on bad
|
||||
* magic, bad metadata_offset, or any rmsgpack_dom_read_into
|
||||
* failure. Reachable on any malformed .rdb the user has, so
|
||||
* the leak compounds across a directory scan.
|
||||
*
|
||||
* Also free the intfstream_t struct itself. intfstream_close
|
||||
* intentionally only closes the inner file (existing libretro-
|
||||
* common convention -- see the trailing 'free(file)' calls in
|
||||
* core_info.c, core_backup.c, cdfs.c, rpng_encode.c which
|
||||
* compensate for this), but libretrodb_open didn't. This was
|
||||
* a 48-byte leak per failed open. */
|
||||
if (db->path)
|
||||
{
|
||||
free(db->path);
|
||||
db->path = NULL;
|
||||
}
|
||||
if (fd)
|
||||
{
|
||||
intfstream_close(fd);
|
||||
free(fd);
|
||||
}
|
||||
return -1;
|
||||
}
|
||||
|
||||
@@ -666,8 +694,13 @@ void libretrodb_cursor_close(libretrodb_cursor_t *cursor)
|
||||
if (!cursor)
|
||||
return;
|
||||
|
||||
/* See libretrodb_close: intfstream_close does not free the
|
||||
* struct. Match the convention. */
|
||||
if (cursor->fd)
|
||||
{
|
||||
intfstream_close(cursor->fd);
|
||||
free(cursor->fd);
|
||||
}
|
||||
|
||||
if (cursor->query)
|
||||
libretrodb_query_free(cursor->query);
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
TARGET := libretrodb_leak_test
|
||||
|
||||
LIBRETRODB_DIR := ../..
|
||||
LIBRETRO_COMM_DIR := ../../../libretro-common
|
||||
|
||||
SOURCES := \
|
||||
libretrodb_leak_test.c \
|
||||
$(LIBRETRODB_DIR)/libretrodb.c \
|
||||
$(LIBRETRODB_DIR)/rmsgpack.c \
|
||||
$(LIBRETRODB_DIR)/rmsgpack_dom.c \
|
||||
$(LIBRETRODB_DIR)/bintree.c \
|
||||
$(LIBRETRODB_DIR)/query.c \
|
||||
$(LIBRETRO_COMM_DIR)/streams/interface_stream.c \
|
||||
$(LIBRETRO_COMM_DIR)/streams/file_stream.c \
|
||||
$(LIBRETRO_COMM_DIR)/streams/memory_stream.c \
|
||||
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c \
|
||||
$(LIBRETRO_COMM_DIR)/compat/compat_fnmatch.c \
|
||||
$(LIBRETRO_COMM_DIR)/compat/fopen_utf8.c \
|
||||
$(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \
|
||||
$(LIBRETRO_COMM_DIR)/encodings/encoding_crc32.c \
|
||||
$(LIBRETRO_COMM_DIR)/file/file_path.c \
|
||||
$(LIBRETRO_COMM_DIR)/file/file_path_io.c \
|
||||
$(LIBRETRO_COMM_DIR)/string/stdstring.c \
|
||||
$(LIBRETRO_COMM_DIR)/time/rtime.c \
|
||||
$(LIBRETRO_COMM_DIR)/vfs/vfs_implementation.c
|
||||
|
||||
OBJS := $(SOURCES:.c=.o)
|
||||
|
||||
CFLAGS += -Wall -pedantic -std=gnu99 -g \
|
||||
-I$(LIBRETRO_COMM_DIR)/include \
|
||||
-I$(LIBRETRODB_DIR)
|
||||
LDFLAGS +=
|
||||
|
||||
# Optional ASan/UBSan build: invoke as 'make SANITIZER=address' or
|
||||
# 'make SANITIZER=address,undefined'. Matches the convention used
|
||||
# in samples/tasks/http/Makefile. Required to actually catch leak
|
||||
# regressions; the default build is a smoke test only.
|
||||
ifneq ($(SANITIZER),)
|
||||
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
|
||||
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
|
||||
endif
|
||||
|
||||
all: $(TARGET)
|
||||
|
||||
%.o: %.c
|
||||
$(CC) -c -o $@ $< $(CFLAGS)
|
||||
|
||||
$(TARGET): $(OBJS)
|
||||
$(CC) -o $@ $^ $(LDFLAGS)
|
||||
|
||||
clean:
|
||||
rm -f $(TARGET) $(OBJS)
|
||||
|
||||
.PHONY: clean
|
||||
@@ -0,0 +1,231 @@
|
||||
/* Regression test for memory leaks in libretrodb_open and
|
||||
* libretrodb_close (libretro-db/libretrodb.c).
|
||||
*
|
||||
* Two pre-this-patch leaks:
|
||||
*
|
||||
* 1. libretrodb_open's error: label freed neither db->path
|
||||
* (strdup'd at line 209) nor the intfstream_t struct (alloc'd
|
||||
* by intfstream_open_file at line 201). Triggered by any
|
||||
* malformed .rdb -- bad magic, bad metadata_offset, truncated
|
||||
* header. Reachable across a directory scan: every malformed
|
||||
* .rdb the user has on disk leaks 48 + (path-length) bytes.
|
||||
*
|
||||
* 2. libretrodb_close (success-path counterpart) called
|
||||
* intfstream_close on db->fd but never freed the struct
|
||||
* itself. Same 48-byte leak per opened-and-closed database
|
||||
* file.
|
||||
*
|
||||
* 3. libretrodb_cursor_close had the same close-without-free
|
||||
* pattern on cursor->fd.
|
||||
*
|
||||
* libretro-common convention is for callers to free the
|
||||
* intfstream_t after intfstream_close (see core_info.c,
|
||||
* core_backup.c, cdfs.c, rpng_encode.c trailing free() calls).
|
||||
* libretrodb's own teardown paths just didn't follow it.
|
||||
*
|
||||
* The test writes a few minimal .rdb files into /tmp, runs them
|
||||
* through libretrodb_open + libretrodb_close cycles, and exits
|
||||
* cleanly. Under -fsanitize=address (the SANITIZER=address
|
||||
* Makefile build) any reintroduced leak is flagged at exit.
|
||||
*
|
||||
* Run with: make SANITIZER=address && ./libretrodb_leak_test
|
||||
*/
|
||||
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
#include <stdint.h>
|
||||
#include <unistd.h>
|
||||
|
||||
#include "../../libretrodb.h"
|
||||
|
||||
#ifndef RETRO_VFS_FILE_ACCESS_READ
|
||||
#define RETRO_VFS_FILE_ACCESS_READ (1 << 0)
|
||||
#endif
|
||||
|
||||
static int failures = 0;
|
||||
|
||||
static void put64be(uint8_t *p, uint64_t v)
|
||||
{
|
||||
int i;
|
||||
for (i = 0; i < 8; i++)
|
||||
p[i] = (uint8_t)(v >> (56 - 8 * i));
|
||||
}
|
||||
|
||||
/* Build a .rdb header at *p. Returns bytes written.
|
||||
* Magic is "RARCHDB\0" (8) + 8-byte big-endian metadata_offset. */
|
||||
static size_t make_header(uint8_t *p, uint64_t metadata_offset)
|
||||
{
|
||||
memcpy(p, "RARCHDB", 7);
|
||||
p[7] = '\0';
|
||||
put64be(p + 8, metadata_offset);
|
||||
return 16;
|
||||
}
|
||||
|
||||
/* Write `len` bytes from `buf` to a temp file at `path`. Returns
|
||||
* 0 on success, -1 on failure. */
|
||||
static int write_temp(const char *path, const uint8_t *buf, size_t len)
|
||||
{
|
||||
FILE *f = fopen(path, "wb");
|
||||
if (!f)
|
||||
return -1;
|
||||
if (fwrite(buf, 1, len, f) != len)
|
||||
{
|
||||
fclose(f);
|
||||
return -1;
|
||||
}
|
||||
fclose(f);
|
||||
return 0;
|
||||
}
|
||||
|
||||
static void test_open_close_valid(void)
|
||||
{
|
||||
/* Minimal valid .rdb: header pointing at a fixmap-1 with
|
||||
* {"count": 0} immediately after. */
|
||||
uint8_t buf[64];
|
||||
const char *path = "/tmp/libretrodb_leak_test_valid.rdb";
|
||||
libretrodb_t *db;
|
||||
int rv;
|
||||
size_t len = make_header(buf, 16);
|
||||
buf[len++] = 0x81; /* fixmap-1 */
|
||||
buf[len++] = 0xa5; memcpy(buf + len, "count", 5); len += 5;
|
||||
buf[len++] = 0x00; /* fixint 0 */
|
||||
if (write_temp(path, buf, len) < 0)
|
||||
{
|
||||
printf("[ERROR] write_temp failed\n"); failures++; return;
|
||||
}
|
||||
|
||||
db = libretrodb_new();
|
||||
if (!db) { printf("[ERROR] libretrodb_new\n"); failures++; goto cleanup; }
|
||||
rv = libretrodb_open(path, db, false);
|
||||
if (rv != 0)
|
||||
{
|
||||
printf("[ERROR] valid .rdb open failed (rv=%d)\n", rv);
|
||||
failures++;
|
||||
}
|
||||
else
|
||||
{
|
||||
libretrodb_close(db);
|
||||
printf("[SUCCESS] valid .rdb open + close cycle clean\n");
|
||||
}
|
||||
libretrodb_free(db);
|
||||
cleanup:
|
||||
unlink(path);
|
||||
}
|
||||
|
||||
static void test_open_bad_magic(void)
|
||||
{
|
||||
/* Bad magic: header starts with 'XXXX...' instead of 'RARCHDB'.
|
||||
* Pre-fix this leaked 48 bytes (intfstream) + 11 bytes
|
||||
* (strdup'd /tmp/...) per call. */
|
||||
uint8_t buf[16];
|
||||
const char *path = "/tmp/libretrodb_leak_test_badmagic.rdb";
|
||||
libretrodb_t *db;
|
||||
int rv;
|
||||
memset(buf, 'X', 8);
|
||||
put64be(buf + 8, 16);
|
||||
if (write_temp(path, buf, sizeof(buf)) < 0)
|
||||
{
|
||||
printf("[ERROR] write_temp failed\n"); failures++; return;
|
||||
}
|
||||
|
||||
db = libretrodb_new();
|
||||
if (!db) { printf("[ERROR] libretrodb_new\n"); failures++; goto cleanup; }
|
||||
rv = libretrodb_open(path, db, false);
|
||||
if (rv == 0)
|
||||
{
|
||||
printf("[ERROR] bad-magic .rdb accepted\n");
|
||||
failures++;
|
||||
libretrodb_close(db);
|
||||
}
|
||||
else
|
||||
printf("[SUCCESS] bad-magic .rdb rejected without leak\n");
|
||||
libretrodb_free(db);
|
||||
cleanup:
|
||||
unlink(path);
|
||||
}
|
||||
|
||||
static void test_open_bad_metadata_offset(void)
|
||||
{
|
||||
/* metadata_offset past EOF: 16-byte file, offset = 100. */
|
||||
uint8_t buf[16];
|
||||
const char *path = "/tmp/libretrodb_leak_test_badoff.rdb";
|
||||
libretrodb_t *db;
|
||||
int rv;
|
||||
make_header(buf, 100);
|
||||
if (write_temp(path, buf, sizeof(buf)) < 0)
|
||||
{
|
||||
printf("[ERROR] write_temp failed\n"); failures++; return;
|
||||
}
|
||||
|
||||
db = libretrodb_new();
|
||||
if (!db) { printf("[ERROR] libretrodb_new\n"); failures++; goto cleanup; }
|
||||
rv = libretrodb_open(path, db, false);
|
||||
if (rv == 0)
|
||||
{
|
||||
printf("[ERROR] bad-offset .rdb accepted\n");
|
||||
failures++;
|
||||
libretrodb_close(db);
|
||||
}
|
||||
else
|
||||
printf("[SUCCESS] bad-offset .rdb rejected without leak\n");
|
||||
libretrodb_free(db);
|
||||
cleanup:
|
||||
unlink(path);
|
||||
}
|
||||
|
||||
static void test_repeat_open_close(void)
|
||||
{
|
||||
/* Open and close the valid .rdb several times on the same
|
||||
* libretrodb_t. libretrodb_open at line ~206-209 frees
|
||||
* db->path if it's already set then strdup's the new one --
|
||||
* which is fine, but the intfstream_t leak compounded across
|
||||
* each call pre-fix. */
|
||||
uint8_t buf[64];
|
||||
const char *path = "/tmp/libretrodb_leak_test_repeat.rdb";
|
||||
libretrodb_t *db;
|
||||
int i;
|
||||
size_t len = make_header(buf, 16);
|
||||
buf[len++] = 0x81;
|
||||
buf[len++] = 0xa5; memcpy(buf + len, "count", 5); len += 5;
|
||||
buf[len++] = 0x00;
|
||||
if (write_temp(path, buf, len) < 0)
|
||||
{
|
||||
printf("[ERROR] write_temp failed\n"); failures++; return;
|
||||
}
|
||||
|
||||
db = libretrodb_new();
|
||||
if (!db) { printf("[ERROR] libretrodb_new\n"); failures++; goto cleanup; }
|
||||
for (i = 0; i < 5; i++)
|
||||
{
|
||||
int rv = libretrodb_open(path, db, false);
|
||||
if (rv != 0)
|
||||
{
|
||||
printf("[ERROR] repeat open #%d failed\n", i);
|
||||
failures++;
|
||||
break;
|
||||
}
|
||||
libretrodb_close(db);
|
||||
}
|
||||
libretrodb_free(db);
|
||||
if (i == 5)
|
||||
printf("[SUCCESS] 5x open+close cycle clean\n");
|
||||
cleanup:
|
||||
unlink(path);
|
||||
}
|
||||
|
||||
int main(void)
|
||||
{
|
||||
test_open_close_valid();
|
||||
test_open_bad_magic();
|
||||
test_open_bad_metadata_offset();
|
||||
test_repeat_open_close();
|
||||
|
||||
if (failures)
|
||||
{
|
||||
printf("\n%d libretrodb leak test(s) failed\n", failures);
|
||||
return 1;
|
||||
}
|
||||
printf("\nAll libretrodb leak regression tests passed.\n");
|
||||
return 0;
|
||||
}
|
||||
@@ -27,6 +27,14 @@ CFLAGS += -Wall -pedantic -std=gnu99 -g \
|
||||
-I$(LIBRETRODB_DIR)
|
||||
LDFLAGS +=
|
||||
|
||||
# Optional ASan/UBSan build: invoke as 'make SANITIZER=address' or
|
||||
# 'make SANITIZER=address,undefined'. Matches the convention used
|
||||
# in samples/tasks/http/Makefile.
|
||||
ifneq ($(SANITIZER),)
|
||||
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
|
||||
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
|
||||
endif
|
||||
|
||||
all: $(TARGET)
|
||||
|
||||
%.o: %.c
|
||||
|
||||
@@ -77,10 +77,12 @@ static int run_dom_read(const uint8_t *data, size_t len)
|
||||
if (rv == 0)
|
||||
rmsgpack_dom_value_free(&v);
|
||||
intfstream_close(fd);
|
||||
/* intfstream_close intentionally does not free the
|
||||
* intfstream_t struct itself (existing libretro-common
|
||||
* convention). This is a documented pre-existing leak;
|
||||
* the test exits before it matters. */
|
||||
/* intfstream_close closes the inner stream but does not free
|
||||
* the intfstream_t struct (existing libretro-common
|
||||
* convention -- see core_info.c, core_backup.c, cdfs.c which
|
||||
* also free after close). ASan flags this as a 48-byte leak
|
||||
* if we don't free it ourselves. */
|
||||
free(fd);
|
||||
return rv;
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user