Skip to content

Commit c82db9f

Browse files
committed
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.
1 parent 7d6477b commit c82db9f

6 files changed

Lines changed: 342 additions & 5 deletions

File tree

.github/workflows/Linux-libretro-db-samples.yml

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,14 @@ jobs:
3333
- name: Build and run samples
3434
shell: bash
3535
working-directory: libretro-db/samples
36+
env:
37+
# Build samples under AddressSanitizer + UndefinedBehaviorSanitizer.
38+
# Both libretro-db sample tests support this via the SANITIZER=
39+
# convention also used in samples/tasks/http/Makefile. ASan
40+
# catches any reintroduced leak (intfstream_t in libretrodb_open
41+
# / cursor_close, etc.) and any out-of-bounds memory access, so
42+
# this is the regression-protective build for these tests.
43+
MAKE_ARGS: "SANITIZER=address,undefined"
3644
run: |
3745
set -u
3846
set -o pipefail
@@ -42,6 +50,7 @@ jobs:
4250
# failure. These are built AND executed.
4351
declare -a RUN_TARGETS=(
4452
rmsgpack_overflow_test
53+
libretrodb_leak_test
4554
)
4655
4756
# Per-binary run command (overrides ./<binary> if present).
@@ -86,7 +95,7 @@ jobs:
8695
fi
8796
8897
# Build
89-
if ! ( cd "$d" && make clean all ); then
98+
if ! ( cd "$d" && make clean all $MAKE_ARGS ); then
9099
printf '\n::error title=Build failed::%s failed to build\n' "$rel"
91100
fails=$((fails+1))
92101
continue

libretro-db/libretrodb.c

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,16 @@ int libretrodb_create(intfstream_t *fd, libretrodb_value_provider value_provider
184184

185185
void libretrodb_close(libretrodb_t *db)
186186
{
187+
/* intfstream_close closes the inner file but does not free
188+
* the intfstream_t struct itself (existing libretro-common
189+
* convention). Match the cleanup pattern used by
190+
* core_info.c / core_backup.c / cdfs.c / rpng_encode.c which
191+
* also free the struct after closing. */
187192
if (db->fd)
193+
{
188194
intfstream_close(db->fd);
195+
free(db->fd);
196+
}
189197
if (db->path && *db->path)
190198
free(db->path);
191199
db->path = NULL;
@@ -246,8 +254,28 @@ int libretrodb_open(const char *path, libretrodb_t *db, bool write)
246254
return 0;
247255

248256
error:
257+
/* Free the strdup'd path (assigned at line 209) on the error
258+
* path: pre-this-commit it was leaked unconditionally on bad
259+
* magic, bad metadata_offset, or any rmsgpack_dom_read_into
260+
* failure. Reachable on any malformed .rdb the user has, so
261+
* the leak compounds across a directory scan.
262+
*
263+
* Also free the intfstream_t struct itself. intfstream_close
264+
* intentionally only closes the inner file (existing libretro-
265+
* common convention -- see the trailing 'free(file)' calls in
266+
* core_info.c, core_backup.c, cdfs.c, rpng_encode.c which
267+
* compensate for this), but libretrodb_open didn't. This was
268+
* a 48-byte leak per failed open. */
269+
if (db->path)
270+
{
271+
free(db->path);
272+
db->path = NULL;
273+
}
249274
if (fd)
275+
{
250276
intfstream_close(fd);
277+
free(fd);
278+
}
251279
return -1;
252280
}
253281

@@ -666,8 +694,13 @@ void libretrodb_cursor_close(libretrodb_cursor_t *cursor)
666694
if (!cursor)
667695
return;
668696

697+
/* See libretrodb_close: intfstream_close does not free the
698+
* struct. Match the convention. */
669699
if (cursor->fd)
700+
{
670701
intfstream_close(cursor->fd);
702+
free(cursor->fd);
703+
}
671704

672705
if (cursor->query)
673706
libretrodb_query_free(cursor->query);
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
TARGET := libretrodb_leak_test
2+
3+
LIBRETRODB_DIR := ../..
4+
LIBRETRO_COMM_DIR := ../../../libretro-common
5+
6+
SOURCES := \
7+
libretrodb_leak_test.c \
8+
$(LIBRETRODB_DIR)/libretrodb.c \
9+
$(LIBRETRODB_DIR)/rmsgpack.c \
10+
$(LIBRETRODB_DIR)/rmsgpack_dom.c \
11+
$(LIBRETRODB_DIR)/bintree.c \
12+
$(LIBRETRODB_DIR)/query.c \
13+
$(LIBRETRO_COMM_DIR)/streams/interface_stream.c \
14+
$(LIBRETRO_COMM_DIR)/streams/file_stream.c \
15+
$(LIBRETRO_COMM_DIR)/streams/memory_stream.c \
16+
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c \
17+
$(LIBRETRO_COMM_DIR)/compat/compat_fnmatch.c \
18+
$(LIBRETRO_COMM_DIR)/compat/fopen_utf8.c \
19+
$(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \
20+
$(LIBRETRO_COMM_DIR)/encodings/encoding_crc32.c \
21+
$(LIBRETRO_COMM_DIR)/file/file_path.c \
22+
$(LIBRETRO_COMM_DIR)/file/file_path_io.c \
23+
$(LIBRETRO_COMM_DIR)/string/stdstring.c \
24+
$(LIBRETRO_COMM_DIR)/time/rtime.c \
25+
$(LIBRETRO_COMM_DIR)/vfs/vfs_implementation.c
26+
27+
OBJS := $(SOURCES:.c=.o)
28+
29+
CFLAGS += -Wall -pedantic -std=gnu99 -g \
30+
-I$(LIBRETRO_COMM_DIR)/include \
31+
-I$(LIBRETRODB_DIR)
32+
LDFLAGS +=
33+
34+
# Optional ASan/UBSan build: invoke as 'make SANITIZER=address' or
35+
# 'make SANITIZER=address,undefined'. Matches the convention used
36+
# in samples/tasks/http/Makefile. Required to actually catch leak
37+
# regressions; the default build is a smoke test only.
38+
ifneq ($(SANITIZER),)
39+
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
40+
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
41+
endif
42+
43+
all: $(TARGET)
44+
45+
%.o: %.c
46+
$(CC) -c -o $@ $< $(CFLAGS)
47+
48+
$(TARGET): $(OBJS)
49+
$(CC) -o $@ $^ $(LDFLAGS)
50+
51+
clean:
52+
rm -f $(TARGET) $(OBJS)
53+
54+
.PHONY: clean
Lines changed: 231 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,231 @@
1+
/* Regression test for memory leaks in libretrodb_open and
2+
* libretrodb_close (libretro-db/libretrodb.c).
3+
*
4+
* Two pre-this-patch leaks:
5+
*
6+
* 1. libretrodb_open's error: label freed neither db->path
7+
* (strdup'd at line 209) nor the intfstream_t struct (alloc'd
8+
* by intfstream_open_file at line 201). Triggered by any
9+
* malformed .rdb -- bad magic, bad metadata_offset, truncated
10+
* header. Reachable across a directory scan: every malformed
11+
* .rdb the user has on disk leaks 48 + (path-length) bytes.
12+
*
13+
* 2. libretrodb_close (success-path counterpart) called
14+
* intfstream_close on db->fd but never freed the struct
15+
* itself. Same 48-byte leak per opened-and-closed database
16+
* file.
17+
*
18+
* 3. libretrodb_cursor_close had the same close-without-free
19+
* pattern on cursor->fd.
20+
*
21+
* libretro-common convention is for callers to free the
22+
* intfstream_t after intfstream_close (see core_info.c,
23+
* core_backup.c, cdfs.c, rpng_encode.c trailing free() calls).
24+
* libretrodb's own teardown paths just didn't follow it.
25+
*
26+
* The test writes a few minimal .rdb files into /tmp, runs them
27+
* through libretrodb_open + libretrodb_close cycles, and exits
28+
* cleanly. Under -fsanitize=address (the SANITIZER=address
29+
* Makefile build) any reintroduced leak is flagged at exit.
30+
*
31+
* Run with: make SANITIZER=address && ./libretrodb_leak_test
32+
*/
33+
34+
#include <stdio.h>
35+
#include <stdlib.h>
36+
#include <string.h>
37+
#include <stdint.h>
38+
#include <unistd.h>
39+
40+
#include "../../libretrodb.h"
41+
42+
#ifndef RETRO_VFS_FILE_ACCESS_READ
43+
#define RETRO_VFS_FILE_ACCESS_READ (1 << 0)
44+
#endif
45+
46+
static int failures = 0;
47+
48+
static void put64be(uint8_t *p, uint64_t v)
49+
{
50+
int i;
51+
for (i = 0; i < 8; i++)
52+
p[i] = (uint8_t)(v >> (56 - 8 * i));
53+
}
54+
55+
/* Build a .rdb header at *p. Returns bytes written.
56+
* Magic is "RARCHDB\0" (8) + 8-byte big-endian metadata_offset. */
57+
static size_t make_header(uint8_t *p, uint64_t metadata_offset)
58+
{
59+
memcpy(p, "RARCHDB", 7);
60+
p[7] = '\0';
61+
put64be(p + 8, metadata_offset);
62+
return 16;
63+
}
64+
65+
/* Write `len` bytes from `buf` to a temp file at `path`. Returns
66+
* 0 on success, -1 on failure. */
67+
static int write_temp(const char *path, const uint8_t *buf, size_t len)
68+
{
69+
FILE *f = fopen(path, "wb");
70+
if (!f)
71+
return -1;
72+
if (fwrite(buf, 1, len, f) != len)
73+
{
74+
fclose(f);
75+
return -1;
76+
}
77+
fclose(f);
78+
return 0;
79+
}
80+
81+
static void test_open_close_valid(void)
82+
{
83+
/* Minimal valid .rdb: header pointing at a fixmap-1 with
84+
* {"count": 0} immediately after. */
85+
uint8_t buf[64];
86+
const char *path = "/tmp/libretrodb_leak_test_valid.rdb";
87+
libretrodb_t *db;
88+
int rv;
89+
size_t len = make_header(buf, 16);
90+
buf[len++] = 0x81; /* fixmap-1 */
91+
buf[len++] = 0xa5; memcpy(buf + len, "count", 5); len += 5;
92+
buf[len++] = 0x00; /* fixint 0 */
93+
if (write_temp(path, buf, len) < 0)
94+
{
95+
printf("[ERROR] write_temp failed\n"); failures++; return;
96+
}
97+
98+
db = libretrodb_new();
99+
if (!db) { printf("[ERROR] libretrodb_new\n"); failures++; goto cleanup; }
100+
rv = libretrodb_open(path, db, false);
101+
if (rv != 0)
102+
{
103+
printf("[ERROR] valid .rdb open failed (rv=%d)\n", rv);
104+
failures++;
105+
}
106+
else
107+
{
108+
libretrodb_close(db);
109+
printf("[SUCCESS] valid .rdb open + close cycle clean\n");
110+
}
111+
libretrodb_free(db);
112+
cleanup:
113+
unlink(path);
114+
}
115+
116+
static void test_open_bad_magic(void)
117+
{
118+
/* Bad magic: header starts with 'XXXX...' instead of 'RARCHDB'.
119+
* Pre-fix this leaked 48 bytes (intfstream) + 11 bytes
120+
* (strdup'd /tmp/...) per call. */
121+
uint8_t buf[16];
122+
const char *path = "/tmp/libretrodb_leak_test_badmagic.rdb";
123+
libretrodb_t *db;
124+
int rv;
125+
memset(buf, 'X', 8);
126+
put64be(buf + 8, 16);
127+
if (write_temp(path, buf, sizeof(buf)) < 0)
128+
{
129+
printf("[ERROR] write_temp failed\n"); failures++; return;
130+
}
131+
132+
db = libretrodb_new();
133+
if (!db) { printf("[ERROR] libretrodb_new\n"); failures++; goto cleanup; }
134+
rv = libretrodb_open(path, db, false);
135+
if (rv == 0)
136+
{
137+
printf("[ERROR] bad-magic .rdb accepted\n");
138+
failures++;
139+
libretrodb_close(db);
140+
}
141+
else
142+
printf("[SUCCESS] bad-magic .rdb rejected without leak\n");
143+
libretrodb_free(db);
144+
cleanup:
145+
unlink(path);
146+
}
147+
148+
static void test_open_bad_metadata_offset(void)
149+
{
150+
/* metadata_offset past EOF: 16-byte file, offset = 100. */
151+
uint8_t buf[16];
152+
const char *path = "/tmp/libretrodb_leak_test_badoff.rdb";
153+
libretrodb_t *db;
154+
int rv;
155+
make_header(buf, 100);
156+
if (write_temp(path, buf, sizeof(buf)) < 0)
157+
{
158+
printf("[ERROR] write_temp failed\n"); failures++; return;
159+
}
160+
161+
db = libretrodb_new();
162+
if (!db) { printf("[ERROR] libretrodb_new\n"); failures++; goto cleanup; }
163+
rv = libretrodb_open(path, db, false);
164+
if (rv == 0)
165+
{
166+
printf("[ERROR] bad-offset .rdb accepted\n");
167+
failures++;
168+
libretrodb_close(db);
169+
}
170+
else
171+
printf("[SUCCESS] bad-offset .rdb rejected without leak\n");
172+
libretrodb_free(db);
173+
cleanup:
174+
unlink(path);
175+
}
176+
177+
static void test_repeat_open_close(void)
178+
{
179+
/* Open and close the valid .rdb several times on the same
180+
* libretrodb_t. libretrodb_open at line ~206-209 frees
181+
* db->path if it's already set then strdup's the new one --
182+
* which is fine, but the intfstream_t leak compounded across
183+
* each call pre-fix. */
184+
uint8_t buf[64];
185+
const char *path = "/tmp/libretrodb_leak_test_repeat.rdb";
186+
libretrodb_t *db;
187+
int i;
188+
size_t len = make_header(buf, 16);
189+
buf[len++] = 0x81;
190+
buf[len++] = 0xa5; memcpy(buf + len, "count", 5); len += 5;
191+
buf[len++] = 0x00;
192+
if (write_temp(path, buf, len) < 0)
193+
{
194+
printf("[ERROR] write_temp failed\n"); failures++; return;
195+
}
196+
197+
db = libretrodb_new();
198+
if (!db) { printf("[ERROR] libretrodb_new\n"); failures++; goto cleanup; }
199+
for (i = 0; i < 5; i++)
200+
{
201+
int rv = libretrodb_open(path, db, false);
202+
if (rv != 0)
203+
{
204+
printf("[ERROR] repeat open #%d failed\n", i);
205+
failures++;
206+
break;
207+
}
208+
libretrodb_close(db);
209+
}
210+
libretrodb_free(db);
211+
if (i == 5)
212+
printf("[SUCCESS] 5x open+close cycle clean\n");
213+
cleanup:
214+
unlink(path);
215+
}
216+
217+
int main(void)
218+
{
219+
test_open_close_valid();
220+
test_open_bad_magic();
221+
test_open_bad_metadata_offset();
222+
test_repeat_open_close();
223+
224+
if (failures)
225+
{
226+
printf("\n%d libretrodb leak test(s) failed\n", failures);
227+
return 1;
228+
}
229+
printf("\nAll libretrodb leak regression tests passed.\n");
230+
return 0;
231+
}

libretro-db/samples/rmsgpack/Makefile

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,14 @@ CFLAGS += -Wall -pedantic -std=gnu99 -g \
2727
-I$(LIBRETRODB_DIR)
2828
LDFLAGS +=
2929

30+
# Optional ASan/UBSan build: invoke as 'make SANITIZER=address' or
31+
# 'make SANITIZER=address,undefined'. Matches the convention used
32+
# in samples/tasks/http/Makefile.
33+
ifneq ($(SANITIZER),)
34+
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
35+
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
36+
endif
37+
3038
all: $(TARGET)
3139

3240
%.o: %.c

0 commit comments

Comments
 (0)