Skip to content

Commit e446242

Browse files
LibretroAdminLibretroAdmin
committed
short-chain strlcpy bound fixes via strlcpy_append
Apply strlcpy_append (commit 78c52ab) to seven sites flagged in the strlcpy-misuse audit, all sharing the same shape: _len = strlcpy(dst, src1, sizeof(dst)); dst[_len] = c1; dst[++_len] = c2; ... strlcpy(dst + _len, src2, sizeof(dst) - _len); None of these had a bound on _len. When src1 was longer than sizeof(dst) - 1, strlcpy returned strlen(src1) >= sizeof(dst) and: - the dst[_len], dst[++_len], ... character writes ran off the end of dst (stack buffer overflow); - the trailing strlcpy got len - _len which underflowed size_t, producing an unbounded copy. Sites: menu/drivers/materialui.c:8090 - "Draw message box". dst is msg[NAME_MAX_LENGTH] which is 128 on Xbox1, 3DS, PSP, PS2, GameCube, Wii, WiiU, PS3, Emscripten and 256 elsewhere. src1 is menu_st->input_dialog_kb_label[256] (always 256 regardless of platform). On the small- NAME_MAX_LENGTH platforms a long label drove _len up to ~255 and the chain wrote up to 127 bytes past the stack frame. Reachable from any input dialog. tasks/task_http.c:495 and tasks/task_http_emscripten.c:312 - download status string. dst is tmp[NAME_MAX_LENGTH]; src1 is the localized "Downloading" string. Same primitive on small-NAME_MAX_LENGTH platforms when a translation pushed the prefix close to the cap. menu/cbs/menu_cbs_sublabel.c:2164 - core backup CRC sublabel. The chain wrote 8 character bytes ("00000000") plus a NUL after the localized "Backup CRC: " prefix. dst is the caller's s/len; if a translation pushed the prefix close to len, the 9 byte writes ran past the caller's buffer. steam/steam.c:449/471/493 - rich-presence content strings. Three near-identical cases (CONTENT_SYSTEM, CONTENT_CORE, CONTENT_SYSTEM_CORE). Each builds "LABEL (CONTENT)" or "LABEL (N/A)" with hand-unrolled writes of " (", "N/A", "/", "A", ")", "\0" at offsets _len + N. dst is content[PATH_MAX_LENGTH] (512 on small platforms, 4096 elsewhere) and the per-character write offsets reached _len + 12 on the SYSTEM_CORE variant; long labels OOB- wrote up to 12 bytes past content. Replace each chain with a sequence of strlcpy_append calls on a single rolling cursor, with the helper handling the bound check at every step. No functional change at the buffer-fits-comfortably end; on small buffers the new code truncates cleanly instead of stack-overflowing. No new tests -- the strlcpy_append regression test from commit 78c52ab covers the helper's contract; these are just additional users.
1 parent 78c52ab commit e446242

5 files changed

Lines changed: 44 additions & 93 deletions

File tree

‎menu/cbs/menu_cbs_sublabel.c‎

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2160,25 +2160,14 @@ static int action_bind_sublabel_core_backup_entry(
21602160
const char *crc = list->list[i].alt
21612161
? list->list[i].alt
21622162
: list->list[i].path;
2163-
/* Set sublabel prefix */
2164-
size_t _len = strlcpy(s, msg_hash_to_str(MENU_ENUM_LABEL_VALUE_CORE_BACKUP_CRC), len);
2165-
2163+
size_t _len = 0;
2164+
strlcpy_append(s, len, &_len,
2165+
msg_hash_to_str(MENU_ENUM_LABEL_VALUE_CORE_BACKUP_CRC));
21662166
/* Add CRC string */
21672167
if (!crc || !*crc)
2168-
{
2169-
s[ _len] = '0';
2170-
s[++_len] = '0';
2171-
s[++_len] = '0';
2172-
s[++_len] = '0';
2173-
s[++_len] = '0';
2174-
s[++_len] = '0';
2175-
s[++_len] = '0';
2176-
s[++_len] = '0';
2177-
s[++_len] = '\0';
2178-
}
2168+
strlcpy_append(s, len, &_len, "00000000");
21792169
else
2180-
strlcpy(s + _len, crc, len - _len);
2181-
2170+
strlcpy_append(s, len, &_len, crc);
21822171
return 1;
21832172
}
21842173

‎menu/drivers/materialui.c‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8087,12 +8087,10 @@ static void materialui_frame(void *data, video_frame_info_t *video_info)
80878087
NULL);
80888088

80898089
/* Draw message box */
8090-
_len = strlcpy(msg, label, sizeof(msg));
8091-
msg[ _len] = '\n';
8092-
msg[++_len] = '\0';
8093-
strlcpy(msg + _len,
8094-
str,
8095-
sizeof(msg) - _len);
8090+
_len = 0;
8091+
strlcpy_append(msg, sizeof(msg), &_len, label);
8092+
strlcpy_append(msg, sizeof(msg), &_len, "\n");
8093+
strlcpy_append(msg, sizeof(msg), &_len, str);
80968094
materialui_render_messagebox(mui,
80978095
p_disp,
80988096
userdata, video_width, video_height,

‎steam/steam.c‎

Lines changed: 25 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -446,82 +446,47 @@ void steam_update_presence(enum presence presence, bool force)
446446
}
447447
break;
448448
case STEAM_RICH_PRESENCE_FORMAT_CONTENT_SYSTEM:
449-
_len = strlcpy(content, label, sizeof(content));
450-
content[_len ] = ' ';
451-
content[_len+1] = '(';
452-
content[_len+2] = '\0';
449+
_len = 0;
450+
strlcpy_append(content, sizeof(content), &_len, label);
451+
strlcpy_append(content, sizeof(content), &_len, " (");
453452
if (core_info)
454453
{
455-
_len += 2;
456-
_len += strlcpy(content + _len, core_info->systemname,
457-
sizeof(content) - _len);
458-
content[_len ] = ')';
459-
content[_len+1] = '\0';
454+
strlcpy_append(content, sizeof(content), &_len,
455+
core_info->systemname);
456+
strlcpy_append(content, sizeof(content), &_len, ")");
460457
}
461458
else
462-
{
463-
content[_len+2] = 'N';
464-
content[_len+3] = '/';
465-
content[_len+4] = 'A';
466-
content[_len+5] = ')';
467-
content[_len+6] = '\0';
468-
}
459+
strlcpy_append(content, sizeof(content), &_len, "N/A)");
469460
break;
470461
case STEAM_RICH_PRESENCE_FORMAT_CONTENT_CORE:
471-
_len = strlcpy(content, label, sizeof(content));
472-
content[_len ] = ' ';
473-
content[_len+1] = '(';
474-
content[_len+2] = '\0';
462+
_len = 0;
463+
strlcpy_append(content, sizeof(content), &_len, label);
464+
strlcpy_append(content, sizeof(content), &_len, " (");
475465
if (core_info)
476466
{
477-
_len += 2;
478-
_len += strlcpy(content + _len, core_info->core_name,
479-
sizeof(content) - _len);
480-
content[_len ] = ')';
481-
content[_len+1] = '\0';
467+
strlcpy_append(content, sizeof(content), &_len,
468+
core_info->core_name);
469+
strlcpy_append(content, sizeof(content), &_len, ")");
482470
}
483471
else
484-
{
485-
content[_len+2] = 'N';
486-
content[_len+3] = '/';
487-
content[_len+4] = 'A';
488-
content[_len+5] = ')';
489-
content[_len+6] = '\0';
490-
}
472+
strlcpy_append(content, sizeof(content), &_len, "N/A)");
491473
break;
492474
case STEAM_RICH_PRESENCE_FORMAT_CONTENT_SYSTEM_CORE:
493-
_len = strlcpy(content, label, sizeof(content));
494-
content[_len ] = ' ';
495-
content[_len+1] = '(';
496-
content[_len+2] = '\0';
475+
_len = 0;
476+
strlcpy_append(content, sizeof(content), &_len, label);
477+
strlcpy_append(content, sizeof(content), &_len, " (");
497478
if (core_info)
498479
{
499-
_len += 2;
500-
_len += strlcpy(content + _len, core_info->systemname,
501-
sizeof(content) - _len);
502-
content[_len ] = ' ';
503-
content[_len+1] = '-';
504-
content[_len+2] = ' ';
505-
_len += 3;
506-
_len += strlcpy(content + _len, core_info->core_name,
507-
sizeof(content) - _len);
508-
content[_len ] = ')';
509-
content[_len+1] = '\0';
480+
strlcpy_append(content, sizeof(content), &_len,
481+
core_info->systemname);
482+
strlcpy_append(content, sizeof(content), &_len, " - ");
483+
strlcpy_append(content, sizeof(content), &_len,
484+
core_info->core_name);
485+
strlcpy_append(content, sizeof(content), &_len, ")");
510486
}
511487
else
512-
{
513-
content[_len+2] = 'N';
514-
content[_len+3] = '/';
515-
content[_len+4] = 'A';
516-
content[_len+5] = ' ';
517-
content[_len+6] = '-';
518-
content[_len+7] = ' ';
519-
content[_len+8] = 'N';
520-
content[_len+9] = '/';
521-
content[_len+10] = 'A';
522-
content[_len+11] = ')';
523-
content[_len+12] = '\0';
524-
}
488+
strlcpy_append(content, sizeof(content), &_len,
489+
"N/A - N/A)");
525490
break;
526491
case STEAM_RICH_PRESENCE_FORMAT_NONE:
527492
default:

‎tasks/task_http.c‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -491,17 +491,16 @@ void* task_push_http_transfer_file(const char* url, bool mute,
491491
s = transfer_data->path;
492492
else
493493
s = url;
494-
495-
_len = strlcpy(tmp, msg_hash_to_str(MSG_DOWNLOADING), sizeof(tmp));
496-
tmp[ _len] = ':';
497-
tmp[++_len] = ' ';
498-
tmp[++_len] = '\0';
494+
_len = 0;
495+
strlcpy_append(tmp, sizeof(tmp), &_len,
496+
msg_hash_to_str(MSG_DOWNLOADING));
497+
strlcpy_append(tmp, sizeof(tmp), &_len, ": ");
499498

500499
if (string_ends_with_size(s, ".index",
501500
strlen(s), STRLEN_CONST(".index")))
502501
s = msg_hash_to_str(MSG_INDEX_FILE);
503502

504-
strlcpy(tmp + _len, s, sizeof(tmp) - _len);
503+
strlcpy_append(tmp, sizeof(tmp), &_len, s);
505504

506505
/* should be using type but some callers now rely on type being ignored */
507506
return task_push_http_transfer_generic_titled(

‎tasks/task_http_emscripten.c‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -308,16 +308,16 @@ void* task_push_http_transfer_file(const char* url, bool mute,
308308
s = transfer_data->path;
309309
else
310310
s = url;
311-
312-
_len = strlcpy(tmp, msg_hash_to_str(MSG_DOWNLOADING), sizeof(tmp));
313-
tmp[ _len] = ' ';
314-
tmp[++_len] = '\0';
311+
_len = 0;
312+
strlcpy_append(tmp, sizeof(tmp), &_len,
313+
msg_hash_to_str(MSG_DOWNLOADING));
314+
strlcpy_append(tmp, sizeof(tmp), &_len, " ");
315315

316316
if (string_ends_with_size(s, ".index",
317317
strlen(s), STRLEN_CONST(".index")))
318318
s = msg_hash_to_str(MSG_INDEX_FILE);
319319

320-
strlcpy(tmp + _len, s, sizeof(tmp) - _len);
320+
strlcpy_append(tmp, sizeof(tmp), &_len, s);
321321

322322
t->title = strdup(tmp);
323323
return t;

0 commit comments

Comments
 (0)