Commit 644d71d
committed
================================================================
Suggested commit message
================================================================
cloud_sync: fix one-byte heap overflow in HTTP failure loggers
The four cloud-sync HTTP failure loggers (webdav, s3 ×2, google_drive)
all wrote a NUL terminator at data->data[data->len] and then printed
data->data with %s. The buffer in data->data is owned by net_http
and is sized exactly to data->len: net_http_update() shrinks the
response buffer with realloc(response->data, response->len) on the
P_DONE transition (net_http.c lines 1441, 1501, 1540), and
net_http_data() returns that pointer plus the same length to the
task layer. The terminator write therefore overflows the heap chunk
by one byte and corrupts the next chunk's metadata.
The corruption is silent at the point of the write. Glibc detects
it later, on an unrelated free() near the same arena -- in practice
the next task_http_transfer_cleanup() during startup, when cloud
sync issues many HTTP requests in quick succession. The result is
malloc_printerr / abort with a backtrace through
task_http_transfer_cleanup that does not point at cloud_sync at all.
Switch all four sites from "NUL-and-print-with-%s" to the
length-bounded form RARCH_WARN("%.*s\n", (int)data->len, ...).
This is local to the broken code, requires no producer-side change,
and removes the implicit "consumer must NUL-terminate, allocator
must oversize" handshake that nothing in the codebase documents.
Other consumers of data->data already pass (data->data, data->len)
together and are unaffected.
Reproducer (ASan, distilled from the call chain above):
char *p = realloc(malloc(64*1024), 57); /* mimic net_http */
memcpy(p, body, 57);
p[57] = 0; /* OOB write */
==116==ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 1 at 0x506000000059 thread T0
#0 webdav_log_http_failure
0 bytes after 57-byte region
Fixes the abort path in #16834, #17474, #17731 (and likely several
other "[CloudSync] crash on startup" reports that share the
task_http_transfer_cleanup → malloc_printerr → abort signature).
================================================================
PR description (for github.com/libretro/RetroArch)
================================================================
Cloud sync triggers a one-byte heap-buffer-overflow on every non-2xx
HTTP response, in the failure logging path shared by webdav, s3, and
google_drive. Glibc usually catches the resulting heap corruption a
few allocations later, on an unrelated `free()`, with a backtrace
that points at `task_http_transfer_cleanup` and gives no hint that
cloud sync is the source.
```
```
Reproduces reliably at startup with cloud sync enabled; goes away
with cloud sync disabled. The user-visible repro from #17474
("crashes upon fetching any server file other than manifest.server")
is the same bug -- a server returning anything that hits the failure
logger.
`net_http_update()` shrinks the response buffer to **exactly**
`response->len` bytes when the body is fully received:
- `libretro-common/net/net_http.c:1441` `realloc(response->data, response->len)`
- `libretro-common/net/net_http.c:1501` same
- `libretro-common/net/net_http.c:1540` same
`net_http_data()` returns that pointer plus the same length, and
`task_http_transfer_handler` puts both into `data->data` /
`data->len`.
The four cloud-sync failure loggers then do:
```c
data->data[data->len] = 0;
RARCH_WARN("%s\n", data->data);
```
That's a one-byte write past the end of the heap allocation. It
corrupts the malloc chunk header of whatever sits next on the
arena. Glibc only screams when something adjacent gets `free()`d.
At startup with cloud sync, that "something adjacent" is
overwhelmingly `task_http_transfer_cleanup()`'s own
`free(data->data)` or `free(data)`, which is what users see in the
backtrace.
The repeating-substring log spam in #16834
(`"Beetle PCE Fast.opt", PCE Fast/Beetle PCE Fast.opt", config/Beetle..."`)
is the same root cause manifesting as a corrupt-read instead of an
abort -- different chunk geometry, same OOB.
Replace `data->data[data->len] = 0; RARCH_WARN("%s\n", data->data)`
with `RARCH_WARN("%.*s\n", (int)data->len, (const char*)data->data)`
at all four sites:
- `network/cloud_sync/webdav.c` `webdav_log_http_failure`
- `network/cloud_sync/s3.c` `s3_log_http_failure`
- `network/cloud_sync/s3.c` `s3_log_multipart_initiate_failure`
- `network/cloud_sync/google_drive.c` `gdrive_log_http_failure`
The fix is local and does not change the producer-side ownership
contract. All other consumers of `data->data` in the tree already
use `(data->data, data->len)` together and are unaffected.
ASan reproducer of the exact write pattern (pre-patch):
```
==116==ERROR: AddressSanitizer: heap-buffer-overflow
WRITE of size 1 at 0x506000000059 thread T0
#0 webdav_log_http_failure
0x506000000059 is located 0 bytes after 57-byte region
allocated by realloc / fake_net_http_response
```
Same harness with the patched logger: clean run, no ASan diagnostic.
Closes #17474. Likely closes #16834, #17731 and other "cloud sync
crash on startup" reports with the same `task_http_transfer_cleanup
→ malloc_printerr → abort` signature; if those reports are actually
the WebDAV "begin failed" / network-down case rather than this one,
they need separate diagnosis but should at least stop crashing.
================================================================1 parent f340815 commit 644d71d
3 files changed
Lines changed: 19 additions & 16 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
172 | 172 | | |
173 | 173 | | |
174 | 174 | | |
| 175 | + | |
| 176 | + | |
175 | 177 | | |
176 | | - | |
177 | | - | |
178 | | - | |
179 | | - | |
| 178 | + | |
180 | 179 | | |
181 | 180 | | |
182 | 181 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
791 | 791 | | |
792 | 792 | | |
793 | 793 | | |
| 794 | + | |
| 795 | + | |
| 796 | + | |
794 | 797 | | |
795 | | - | |
796 | | - | |
797 | | - | |
798 | | - | |
| 798 | + | |
799 | 799 | | |
800 | 800 | | |
801 | 801 | | |
| |||
1246 | 1246 | | |
1247 | 1247 | | |
1248 | 1248 | | |
1249 | | - | |
1250 | | - | |
1251 | | - | |
1252 | | - | |
| 1249 | + | |
| 1250 | + | |
1253 | 1251 | | |
1254 | 1252 | | |
1255 | 1253 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
448 | 448 | | |
449 | 449 | | |
450 | 450 | | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
451 | 460 | | |
452 | | - | |
453 | | - | |
454 | | - | |
455 | | - | |
| 461 | + | |
456 | 462 | | |
457 | 463 | | |
458 | 464 | | |
| |||
0 commit comments