Fixed server crash via @reloadpcdb#3377
Conversation
hemagx
left a comment
There was a problem hiding this comment.
From your asan report (I would prefer a crash dump), it says it was a use after free, which means it was free'd else where, why it's not being nulled there and why it's a seperate function? I need more explanation here please.
Why not nulled in pc_clear_exp_groups where the free happens? pc_clear_exp_groups only owns and manages the VECTOR storage — it frees the exp arrays inside each class_exp_group entry and clears the vectors. It has no knowledge of class_exp_table, and by design it shouldn't: class_exp_table is a separate non-owning pointer array that acts as a class-indexed lookup into those vectors, and it is populated not by any function in pc.c but by status->read_job_db() in status.c. The two are in different abstraction layers — one owns the data, the other indexes it. Coupling pc_clear_exp_groups to nulling a lookup table it doesn't populate would mean it needs to know about a side effect that belongs to a different module. Why a separate function? Because the clear of class_exp_table needs to explicitly mirror its repopulation by status->read_job_db(). In pc_readdb the sequence is now: clear the lookup → free the storage → reload storage → repopulate the lookup. Making it a named function pointer in the pc interface keeps that intent readable and also follows the existing HPM hook pattern of the codebase, so plugins can override it if needed. An alternative would have been to inline the null loop directly in pc_readdb, but a named function makes the symmetry with read_job_db more obvious. |
Pull Request Prelude
Changes Proposed
Issues addressed: #2811