Repository navigation
Conversation
rc_preparse_reserve_memrefs() linked a new list into the pool even when its items could not be allocated, so later memrefs were written to address 0. It also linked the new list after the first list with room left, which dropped the lists after that one. rc_alloc_memref() and rc_alloc_modified_memref() used a scratch list that failed to allocate, and rc_preparse_copy_memrefs() went on copying after an error.
…ations rc_alloc() and rc_alloc_scratch() went on once the offset held an error: the size was added to the error code, and in the second pass the next object was placed in front of the buffer. A failed allocation of the first scratch instance of a type was not reported, and rc_alloc_str() left a node without a value in the tree. The parsers check the result of RC_ALLOC where they use it right away.
…emory A failed realloc overwrote the triggers or lboards array with NULL, and the capacity grew before the allocation succeeded. The early returns of the three activate functions leaked the preparse state, and a failed rich presence buffer left a half initialized entry behind. A failed memref reservation is checked before the second parse.
Jamiras
left a comment
Jamiras
left a comment
There was a problem hiding this comment.
I think these changes look okay. I did leave a comment recommending the comments suggesting the return value state be removed.
I'll do a more thorough review once the merge conflicts are resolved (sorry about that).
| memref_list->items = RC_ALLOC_ARRAY_SCRATCH(rc_memref_t, 8, parse); | ||
| if (!memref_list->items) { | ||
| memref_list->count = memref_list->capacity = 0; | ||
| return NULL; /* parse->offset is RC_OUT_OF_MEMORY */ |
There was a problem hiding this comment.
Comment should be in the if, not the return:
if (!memref_list->items) { /* parse->offset is RC_OUT_OF_MEMORY */
memref_list->count = memref_list->capacity = 0;
return NULL;
}
But I'd lean towards not having the comment at all. It might also be RC_INSUFFICIENT_BUFFER
There was a problem hiding this comment.
Same comment applies to several other locations in this PR.
The conflicts in rc_alloc.c and rc_richpresence.c come from RetroAchievements#557: the checks are now made in the new rc_alloc() and rc_alloc_scratch(). Since RetroAchievements#557 the scratch pointer array is allocated on first use, so the first RC_ALLOC of a preparse can fail too: the root objects are checked before they are parsed.
|
Thanks! I merged develop in and resolved the conflicts. Since #557 the scratch pointer array is allocated on first use, so the first RC_ALLOC of a preparse can fail as well, and the root objects are now checked before they are parsed. The return value comments are gone (22de37a). The fault injection run is clean again on the merged code, and the test suite passes. |
This branch has not been deployed
2 participants
When
malloc,callocorreallocfails while a set is activated, the runtime usually crashes or leaks. The worst case isrc_preparse_reserve_memrefs(): it links a memref list whoseitemsfailed to allocate, so the memrefs of later activations are written to address 0.With this change every failed allocation ends in
RC_OUT_OF_MEMORYfor that one item, and the runtime stays usable. There are three commits: the memref lists, the parse state after an error (nothing more is allocated once the offset holds an error, and the result ofRC_ALLOCis checked where it is used right away), and the activate functions inrc_runtime.c(realloc, capacity, preparse state, rich presence).Tested with a harness that fails the n-th allocation, once for every n, under ASan and LSan. The input was 40 achievements, 40 leaderboards and a rich presence script with a lookup, and separately two real sets. Before the change, 316 of 320 and 272 of 355 failure points crashed or leaked. After it there are none, and activating everything again afterwards gives the same events as a run without a failure. The test suite passes as before (x86, x64, valgrind).
rc_runtime_init()doesn't check itsmalloceither. That would need an API change, so it's left as is. I can add the harness as a test if you like.Found on an RP2350 (Pico 2 W), where a set activated next to a parsed one ran out of heap.