Bound every array a data file can index
Closes Defects items 16 and 17 and Known-and-still-open item 6. All three let an asset file, or a caller's argument, write past a fixed array. akgl_sprite_load_json took its frame count straight from the document and wrote that many entries into a 16-byte frameids -- through a uint32_t * cast of a uint8_t *, so each write touched four bytes and the overrun reached four bytes past the array, into the rest of akgl_Sprite and then the next pool slot. The count is checked first now, each id is read into an int and narrowed deliberately, and a frame number too large for a uint8_t is refused rather than truncated into an index for a different tile. The tilemap loader had the same shape twice: objects[j] with no check against AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER and tilesets[i] with none against AKGL_TILEMAP_MAX_TILESETS. akgl_tilemap_load_layers already bounded its own loop, so the pattern was in the same file. The object one is the reachable half -- 128 objects is not a large object layer. akgl_string_initialize zeroed sizeof(akgl_String) starting at `data`, which begins after the refcount in front of it, so it ran four bytes past the end of the object and onto the *next* slot's refcount -- the field the allocator reads to decide whether a slot is free. Same file, same class, fixed with it: akgl_string_copy accepted a count larger than the buffers, reading past one pool slot and writing past another, which the header documented as behaviour. Every case has a test that fails against the old code, with five new fixtures. Exactly-the-maximum is asserted alongside one-past in each, so the bound cannot be fixed by making the limit off by one. 25/25 pass, memcheck clean, reindent --check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
55
TODO.md
55
TODO.md
@@ -678,9 +678,19 @@ Each was found by a test written to assert correct behavior.
|
||||
`src/util.c:208` compares `s1->pitch * s1->h` bytes of `s2` without verifying
|
||||
the surfaces share dimensions, pitch, or format.
|
||||
6. **`akgl_string_initialize` overflows by four bytes when `init` is NULL.**
|
||||
`src/staticstring.c:17` does `memset(&obj->data, 0x00, sizeof(akgl_String))`,
|
||||
but `data` starts four bytes into the struct (after `refcount`), so the memset
|
||||
runs four bytes past the end.
|
||||
**Fixed in 0.5.0**: it zeroes `sizeof(obj->data)`. The four bytes it used to
|
||||
run past the end of the object landed on the *next* pool slot's `refcount`,
|
||||
which is the field the allocator reads to decide whether a slot is free --
|
||||
so the overrun could hand a live string out twice.
|
||||
|
||||
`tests/staticstring.c` claims two adjacent slots, stamps a sentinel into the
|
||||
second's refcount, and initializes the first.
|
||||
|
||||
Fixed alongside it, because it is the same file and the same class:
|
||||
`akgl_string_copy` accepted a `count` above `AKGL_MAX_STRING_LENGTH`, which
|
||||
read past the end of one pool slot and wrote past the end of another. The
|
||||
header documented that as behaviour. It is `AKERR_OUTOFBOUNDS` now, and a
|
||||
negative count is refused too.
|
||||
7. **Savegame name lengths disagree between writer and reader.**
|
||||
`akgl_game_save_actors` writes spritesheet names at
|
||||
`AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` (512) and character names at
|
||||
@@ -819,35 +829,24 @@ without coming here first. Ordered by blast radius.
|
||||
only record that a retry is wanted. Touches `src/util.c:117-121` only.
|
||||
|
||||
16. **`akgl_sprite_load_json` does not bound the `frames` array.**
|
||||
`src/sprite.c:165` sets `obj->frames` from `json_array_size()` and
|
||||
`src/sprite.c:167` then writes that many entries into
|
||||
`frameids[AKGL_SPRITE_MAX_FRAMES]`, which is 16. A sprite definition with 17
|
||||
or more frames writes past the array into the rest of `akgl_Sprite`, and
|
||||
past the struct into the neighbouring pool slot. It is also written through
|
||||
a `uint32_t *` cast of a `uint8_t *`, so each element write touches four
|
||||
bytes; that happens to work on a little-endian machine because the following
|
||||
iterations overwrite the spill and the last write lands in the struct's
|
||||
alignment padding, but it is not portable and it is what makes the overflow
|
||||
reach four bytes past `frames` rather than one.
|
||||
**Fixed in 0.5.0.** The count is bounded against `AKGL_SPRITE_MAX_FRAMES`
|
||||
before anything is written, and each element is read into an `int` and
|
||||
narrowed deliberately rather than written through a `uint32_t *` cast of a
|
||||
`uint8_t *`. A frame number that does not fit a `uint8_t` is refused too,
|
||||
rather than truncated into an index that names a different tile.
|
||||
|
||||
Fix: refuse a `frames` array longer than `AKGL_SPRITE_MAX_FRAMES` with
|
||||
`AKERR_OUTOFBOUNDS`, and read into an `int` local before narrowing to
|
||||
`uint8_t`. Touches `src/sprite.c:164-169`.
|
||||
`tests/sprite.c` covers exactly the maximum (must load, and every id must
|
||||
arrive), one past it, and the wide frame number, against three new fixtures.
|
||||
Against the old code the middle case loads happily.
|
||||
|
||||
17. **Two more unbounded array loads in the tilemap loader.**
|
||||
`src/tilemap.c:387-389` walks a Tiled object layer straight into
|
||||
`curlayer->objects[j]` with no check against
|
||||
`AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER` (128), and `src/tilemap.c:278-280`
|
||||
fills `dest->tilesets[i]` with no check against `AKGL_TILEMAP_MAX_TILESETS`
|
||||
(16).
|
||||
Both are the same shape as item 16 and both are reachable from an ordinary
|
||||
map file: 128 objects is not a large object layer. Note that
|
||||
`akgl_tilemap_load_layers` *does* bound its loop and raises
|
||||
`AKERR_OUTOFBOUNDS`, so the pattern to copy is already in the same file.
|
||||
**Fixed in 0.5.0**, both with the bound at the top of the loop body, the
|
||||
shape `akgl_tilemap_load_layers` in the same file already used.
|
||||
|
||||
Fix: add the bound check at the top of each loop body, matching
|
||||
`akgl_tilemap_load_layers`. Touches `src/tilemap.c:387` and
|
||||
`src/tilemap.c:278`.
|
||||
`tests/tilemap.c` covers both against generated fixtures: an object layer
|
||||
of exactly 128 (must load) and of 129, and a map with 17 tilesets. The
|
||||
object one is the reachable half -- 128 objects is not a large object layer
|
||||
and `akgl_TilemapObject` is not small.
|
||||
|
||||
18. **`akgl_get_json_with_default` defaults on a status the array accessors never
|
||||
raise.** `src/json_helpers.c:164-165` handles `AKERR_KEY` and `AKERR_INDEX`,
|
||||
|
||||
Reference in New Issue
Block a user