Skip to content

Commit 0e64839

Browse files
committed
libretro-common: clamp word_wrap_wideglyph return to bytes-written + CI
word_wrap_wideglyph violated its implicit contract that the returned length always fits inside the destination buffer. Three return paths forwarded strlcpy()'s return value, which is strlen(src) per spec -- not bytes-actually-written -- so on truncation the function returned a value larger than the destination it had written into. The sister function word_wrap() has a `len < src_len + 1` guard up front that bails to 0 when the buffer is too small, sidestepping this entirely. word_wrap_wideglyph has no such guard and tries to fit what it can, which is correct behaviour, but the inflated return breaks every caller that uses it as a length argument. Concrete impact: three menu drivers (xmb, ozone, materialui) feed this return value as the `n` argument to memchr() over the wrapped destination buffer when laying out messageboxes (xmb_render_messagebox_internal at xmb.c line ~1183; ozone_draw_messagebox at ozone.c line ~7362; materialui_render_messagebox at materialui.c line ~2825). Pre-patch, an inflated `n` walks memchr past the destination into adjacent stack memory (the lines[] / line_lengths[] arrays the messagebox helpers keep right next to the wrapped buffer). A '\n' byte found there yields a wild pointer used in pointer arithmetic and as a length argument to font_driver_get_message_width(), causing stack info disclosure or a crash. The wideglyph path is selected by msg_hash_get_wideglyph_str(), so the bug is reachable in CJK locales (Japanese, Korean, Chinese) via any messagebox payload exceeding MENU_LABEL_MAX_LENGTH (256 bytes default) -- error notifications, file paths, network handshake text, search results. Two issues fixed in stdstring.c: * Return value: every truncating return path (`return strlcpy(...)` and `return (s - s_start) + strlcpy(...)`) now clamps strlcpy's return to the actual bytes written: copied = strlcpy(s, src, n); if (copied >= n) copied = (n > 0) ? n - 1 : 0; return [(s - s_start) +] copied; This avoids introducing a strnlen() dependency (not used anywhere else in libretro-common, portability concerns on old MSVC). * `len` semantic drift: the loop body decremented `len` by char_len on every byte written, while the early-return paths computed `remaining = len - (s - s_start)`. Both expressions track "bytes used", so the formula effectively double-subtracted, yielding a too-small `remaining` and forcing the strlcpys to truncate harder than necessary. The lastspace/lastwideglyph rewinds at lines ~411 and ~429 also moved `s` backward without a matching `len` adjustment, desyncing the two over time. Drop the `len -= char_len`, compute `remaining = len - (s - s_start)` once at the top of each loop iteration (and reuse it for the buffer-overflow guard previously spelled `if (char_len >= len)`). `len` now stays constant across the function body, matching word_wrap()'s convention. Regression test: libretro-common/samples/string/word_wrap_overflow _test/. Five cases covering both truncating and non-truncating inputs across the line-358 early-return path and the main-loop late-return paths. The discriminator is two-fold: the test asserts return-value-vs-bytes-written directly, AND mimics the menu drivers' call shape by feeding the returned value to memchr() over the destination, so an ASan build catches pre-patch failures via heap-buffer-overflow on the memchr. Verified by running against both pre-patch and post-patch stdstring.c: pre-patch: short_input_tiny_dest fails (returned=47, dst_size=16) -> exit 1 post-patch: all five cases pass -> exit 0 CI: word_wrap_overflow_test added to RUN_TARGETS in .github/workflows/Linux-libretro-common-samples.yml. Builds under SANITIZER=address,undefined like the other overflow regression tests in the same workflow (rpng_chunk_overflow_test, vfs_read_overflow_test, cdrom_cuesheet_overflow_test, chd_meta_overflow_test, cdfs_dir_record_test, rzip_chunk_size_test). Note on libretro-common/test/string/test_stdstring.c (the libcheck-based suite): I considered adding a unit-test entry there too, but the file is already broken at HEAD on master -- test_string_replacesubstr calls string_replace_substring with the old 3-arg signature, but the API was extended to 5 args some time ago; the test fails to compile. Wiring up Makefile.test in CI would have caught that regression but is out of scope here. The samples-based regression test is the working surface and matches the established pattern for every other overflow test in the tree.
1 parent d56c728 commit 0e64839

4 files changed

Lines changed: 346 additions & 9 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ jobs:
7676
rbmp_test
7777
rpng_chunk_overflow_test
7878
rpng_roundtrip_test
79+
word_wrap_overflow_test
7980
)
8081
8182
# Per-binary run command (overrides ./<binary> if present).
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
TARGET := word_wrap_overflow_test
2+
3+
LIBRETRO_COMM_DIR := ../../..
4+
5+
# word_wrap_wideglyph lives in libretro-common/string/stdstring.c.
6+
# Its only non-libc dependency is utf8skip from encoding_utf.c
7+
# (used to walk UTF-8 codepoint boundaries in the input string).
8+
SOURCES := \
9+
word_wrap_overflow_test.c \
10+
$(LIBRETRO_COMM_DIR)/string/stdstring.c \
11+
$(LIBRETRO_COMM_DIR)/encodings/encoding_utf.c \
12+
$(LIBRETRO_COMM_DIR)/compat/compat_strl.c \
13+
$(LIBRETRO_COMM_DIR)/compat/compat_posix_string.c
14+
15+
OBJS := $(SOURCES:.c=.o)
16+
17+
CFLAGS += -Wall -pedantic -std=gnu99 -g -O0 -I$(LIBRETRO_COMM_DIR)/include
18+
19+
ifneq ($(SANITIZER),)
20+
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
21+
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
22+
endif
23+
24+
all: $(TARGET)
25+
26+
%.o: %.c
27+
$(CC) -c -o $@ $< $(CFLAGS)
28+
29+
$(TARGET): $(OBJS)
30+
$(CC) -o $@ $^ $(LDFLAGS)
31+
32+
clean:
33+
rm -f $(TARGET) $(OBJS)
34+
35+
.PHONY: clean
Lines changed: 271 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,271 @@
1+
/* Regression test for return-value-exceeds-bytes-written in
2+
* word_wrap_wideglyph (libretro-common/string/stdstring.c).
3+
*
4+
* Pre-patch, word_wrap_wideglyph had three classes of return path
5+
* that propagated strlcpy()'s return value directly, e.g.:
6+
*
7+
* if (src_end - src < line_width)
8+
* return strlcpy(s, src, len);
9+
*
10+
* ...
11+
*
12+
* return (size_t)(s - s_start) + strlcpy(s, src, remaining);
13+
*
14+
* strlcpy() returns strlen(src), not bytes-actually-written, so on
15+
* truncation (destination smaller than source) the returned value
16+
* exceeds the number of bytes written into the destination buffer.
17+
*
18+
* The sister function word_wrap() has a `len < src_len + 1` guard
19+
* up front that bails to 0 when the buffer is too small, sidestepping
20+
* the issue. word_wrap_wideglyph has no such guard and tries to fit
21+
* what it can -- which is correct behaviour, but the inflated return
22+
* value violates the implicit contract every caller in the tree
23+
* depends on.
24+
*
25+
* Concrete impact: three menu drivers (xmb, ozone, materialui) feed
26+
* this return value as the `n` argument to memchr() over the wrapped
27+
* destination buffer, looking for newline boundaries when laying out
28+
* messageboxes. Pre-patch, an inflated `n` walks memchr past the
29+
* buffer end into adjacent stack memory. A '\n' (0x0A) byte found
30+
* there yields a wild pointer used in pointer arithmetic and as a
31+
* length argument to font_driver_get_message_width(), leading to
32+
* stack info disclosure or a crash. Reachable in CJK locales
33+
* (which select word_wrap_wideglyph via msg_hash_get_wideglyph_str)
34+
* via any messagebox payload that exceeds MENU_LABEL_MAX_LENGTH
35+
* (default 256 bytes) -- error notifications, file paths, network
36+
* handshake text, search results.
37+
*
38+
* Post-patch, every return path computes bytes-actually-written
39+
* from strlcpy's contract:
40+
*
41+
* copied = strlcpy(s, src, n);
42+
* if (copied >= n) copied = (n > 0) ? n - 1 : 0;
43+
* return ... + copied;
44+
*
45+
* so the returned value is always a valid offset into the
46+
* destination buffer. This test asserts that invariant directly
47+
* and additionally exercises the call shape used by the menu
48+
* drivers (memchr over the destination using the returned length),
49+
* so ASan flags pre-patch builds with heap-buffer-overflow.
50+
*
51+
* The test uses heap allocation (not a stack buffer) so ASan's
52+
* red-zone instrumentation gives a sharp signal when the bug is
53+
* present. Without ASan, the test still detects the bug on its
54+
* own via the return-value-exceeds-buffer-size assertion.
55+
*/
56+
57+
#include <stdio.h>
58+
#include <stdlib.h>
59+
#include <string.h>
60+
61+
#include <string/stdstring.h>
62+
63+
static int failures = 0;
64+
65+
/* A long ASCII source with no embedded newlines or wide glyphs.
66+
* "ASCII through wideglyph" is the worst-case for this function:
67+
* line 358's early-return branch fires only when the entire input
68+
* is shorter than line_width, so we must use input >= line_width
69+
* to drive flow through the main loop and the late-return paths
70+
* at lines 384 / 420 / 438. The rewinds happen at every space.
71+
*/
72+
static const char *long_src =
73+
"Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam "
74+
"nec enim quis orci euismod efficitur at nec arcu. Vivamus "
75+
"imperdiet est feugiat massa rhoncus porttitor at vitae ante. "
76+
"Nunc a orci vel ipsum tempor posuere sed a lacus. Ut erat "
77+
"odio, ultrices vitae iaculis fringilla, iaculis ut eros. Sed "
78+
"facilisis viverra lectus et ullamcorper. Aenean risus ex, "
79+
"ornare eget scelerisque ac, imperdiet eu ipsum. Morbi "
80+
"pellentesque erat metus, sit amet aliquet libero rutrum et. "
81+
"Integer non ullamcorper tellus.";
82+
83+
static void check_return_value(const char *case_name,
84+
char *dst, size_t dst_size, size_t returned,
85+
const char *src)
86+
{
87+
/* Returned length must NEVER exceed bytes-actually-written.
88+
* Bytes written is at most dst_size - 1 (room for the NUL). */
89+
if (returned >= dst_size)
90+
{
91+
printf("[ERROR] %s: word_wrap_wideglyph returned %zu, "
92+
"destination buffer is only %zu bytes (cannot exceed "
93+
"%zu bytes-written)\n",
94+
case_name, returned, dst_size,
95+
dst_size > 0 ? dst_size - 1 : 0);
96+
failures++;
97+
return;
98+
}
99+
100+
/* Returned length must equal strlen of the destination -- this
101+
* is the contract menu-driver callers depend on. */
102+
{
103+
size_t actual = strlen(dst);
104+
if (returned != actual)
105+
{
106+
printf("[ERROR] %s: word_wrap_wideglyph returned %zu but "
107+
"strlen(dst) is %zu\n",
108+
case_name, returned, actual);
109+
failures++;
110+
return;
111+
}
112+
}
113+
114+
/* Mimic the ozone/xmb/materialui messagebox call shape: use the
115+
* returned value as the `n` argument to memchr() over the
116+
* destination buffer. Pre-patch, an inflated returned value
117+
* walks memchr past the buffer. ASan flags this as a heap-
118+
* buffer-overflow. Post-patch, the read stays within dst. */
119+
{
120+
const char *nl = (const char *)memchr(dst, '\n', returned);
121+
(void)nl; /* presence/absence not asserted; the read itself
122+
* is the test (ASan is the discriminator) */
123+
}
124+
125+
/* (void) src to suppress unused-parameter when not in debug. */
126+
(void)src;
127+
128+
printf("[SUCCESS] %s: returned=%zu, strlen(dst)=%zu, dst_size=%zu\n",
129+
case_name, returned, strlen(dst), dst_size);
130+
}
131+
132+
/* Case 1: destination smaller than line_width.
133+
* Pre-patch this hits the `src_end - src < line_width` branch?
134+
* No -- src_len > line_width, so we reach the main loop. The
135+
* rewinds at lastspace cause early returns at lines 384/420/438
136+
* via strlcpy with a too-small `remaining`. */
137+
static void test_truncating_normal_case(void)
138+
{
139+
const size_t dst_size = 64;
140+
char *dst = (char *)malloc(dst_size);
141+
size_t returned;
142+
143+
if (!dst) { printf("[FATAL] OOM\n"); failures++; return; }
144+
145+
memset(dst, 0xCC, dst_size); /* poison so strlen catches non-NUL-termination */
146+
dst[dst_size - 1] = '\0'; /* ensure strlen() terminates if NUL is missing */
147+
148+
returned = word_wrap_wideglyph(dst, dst_size, long_src,
149+
strlen(long_src),
150+
/* line_width */ 40,
151+
/* wideglyph_width */ 100,
152+
/* max_lines */ 0);
153+
154+
check_return_value("truncating_normal", dst, dst_size, returned, long_src);
155+
free(dst);
156+
}
157+
158+
/* Case 2: very small destination (smaller than even one line
159+
* worth of source). Pre-patch this still hits the buggy path
160+
* because the loop's char_len-vs-len check at line 367 stopped
161+
* the loop, leaving s_start..s short, but the late strlcpy
162+
* (whichever fired first) still returned a too-large value. */
163+
static void test_truncating_tiny_dest(void)
164+
{
165+
const size_t dst_size = 16;
166+
char *dst = (char *)malloc(dst_size);
167+
size_t returned;
168+
169+
if (!dst) { printf("[FATAL] OOM\n"); failures++; return; }
170+
171+
memset(dst, 0xCC, dst_size);
172+
dst[dst_size - 1] = '\0';
173+
174+
returned = word_wrap_wideglyph(dst, dst_size, long_src,
175+
strlen(long_src), 40, 100, 0);
176+
177+
check_return_value("truncating_tiny_dest", dst, dst_size,
178+
returned, long_src);
179+
free(dst);
180+
}
181+
182+
/* Case 3: destination is exactly large enough for the whole
183+
* wrapped output. Tests that the fix doesn't regress the
184+
* non-truncating case -- return value should equal strlen(dst). */
185+
static void test_fits(void)
186+
{
187+
const size_t dst_size = 1024;
188+
char *dst = (char *)malloc(dst_size);
189+
size_t returned;
190+
191+
if (!dst) { printf("[FATAL] OOM\n"); failures++; return; }
192+
193+
memset(dst, 0xCC, dst_size);
194+
dst[dst_size - 1] = '\0';
195+
196+
returned = word_wrap_wideglyph(dst, dst_size, long_src,
197+
strlen(long_src), 40, 100, 0);
198+
199+
check_return_value("fits", dst, dst_size, returned, long_src);
200+
free(dst);
201+
}
202+
203+
/* Case 4: input shorter than line_width -- exercises the early
204+
* return at line 358 (the simplest of the buggy paths). Pre-
205+
* patch, this returns strlen(short_src), which is fine when the
206+
* destination is large enough. This case verifies post-patch
207+
* doesn't regress the easy path. */
208+
static void test_short_input_large_dest(void)
209+
{
210+
const char *short_src = "Hello, world.";
211+
const size_t dst_size = 64;
212+
char *dst = (char *)malloc(dst_size);
213+
size_t returned;
214+
215+
if (!dst) { printf("[FATAL] OOM\n"); failures++; return; }
216+
217+
memset(dst, 0xCC, dst_size);
218+
dst[dst_size - 1] = '\0';
219+
220+
returned = word_wrap_wideglyph(dst, dst_size, short_src,
221+
strlen(short_src), 40, 100, 0);
222+
223+
check_return_value("short_input_large_dest", dst, dst_size,
224+
returned, short_src);
225+
free(dst);
226+
}
227+
228+
/* Case 5: input shorter than line_width AND destination too small.
229+
* This is the line-358 truncation case: pre-patch returns
230+
* strlen(src) which exceeds dst_size; post-patch clamps to dst_size-1. */
231+
static void test_short_input_tiny_dest(void)
232+
{
233+
const char *src = "Hello, world! This is a moderately long string.";
234+
const size_t dst_size = 16; /* smaller than src */
235+
char *dst = (char *)malloc(dst_size);
236+
size_t returned;
237+
238+
if (!dst) { printf("[FATAL] OOM\n"); failures++; return; }
239+
240+
memset(dst, 0xCC, dst_size);
241+
dst[dst_size - 1] = '\0';
242+
243+
/* line_width chosen larger than src_len so the function takes
244+
* the line-358 early-return path. */
245+
returned = word_wrap_wideglyph(dst, dst_size, src,
246+
strlen(src),
247+
/* line_width */ 100, /* > strlen(src) */
248+
100, 0);
249+
250+
check_return_value("short_input_tiny_dest", dst, dst_size,
251+
returned, src);
252+
free(dst);
253+
}
254+
255+
int main(void)
256+
{
257+
test_truncating_normal_case();
258+
test_truncating_tiny_dest();
259+
test_fits();
260+
test_short_input_large_dest();
261+
test_short_input_tiny_dest();
262+
263+
if (failures)
264+
{
265+
printf("\n%d word_wrap_wideglyph regression test(s) failed\n",
266+
failures);
267+
return 1;
268+
}
269+
printf("\nAll word_wrap_wideglyph regression tests passed.\n");
270+
return 0;
271+
}

0 commit comments

Comments
 (0)