Repository navigation
seekable: fix seek table leaks on reinitialization and read failure - #4910
perfloop-agent wants to merge 1 commit into
Conversation
Signed-off-by: Perfloop Agent <agent@perfloop.ai> Signed-off-by: Tomás Senart <ts@perfloop.ai>
|
Hi @perfloop-agent! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Hi, I'm Tomás, I build Perfloop. I approved this contribution and will handle review feedback here. @Cyan4973, this fixes two memory leaks in the same seekable-format module as #4902 and #4717. Successful reinitialization with ZSTD_seekable_initBuff, ZSTD_seekable_initFile or ZSTD_seekable_initAdvanced leaks the previous seek table, and a failed refill read after allocation leaks the new entries. This change frees the previous table when installing a replacement and frees the unpublished entries on refill failure. We found it while hunting correctness bugs with Perfloop. We've had changes merged in Redis, Dolt, Prometheus, ClickHouse, Distribution, and more. If you'd like anything in this patch changed, just let me know and I'll update it. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Closing in favor of #4941, which covers both leaks and the reset contract. Thanks! |
What and why
ZSTD_seekable_loadSeekTable()now frees the installed entries immediately before replacing them with a fully loaded table. It also frees the newly allocated entries when a later refill read fails, returning the same I/O error.This fixes lost seek-table allocations when a decoder is reused or a multi-read load fails. Failed loads keep the previously installed table's metadata, and exported seek tables remain independent copies. The initialization APIs' source-switching behavior is unchanged. Re-initialization frequency in applications has not been measured.
The test in
contrib/seekable_format/tests/seekable_tests.creuses a decoder across successful and failed loads, retries with a smaller archive, and checks an exported table after decoder destruction. It exercises refill failures with and without checksums.The seekable tests passed in normal and AddressSanitizer/UndefinedBehaviorSanitizer builds. The parallel-compression consumer test also passed. Focused clang static analysis reported no bugs.
Checklist
Tools used
AI tools were used to investigate ownership, implement the fix, and write the tests.
Case and evidence
Made by Perfloop. Human sponsor: Tomás Senart. To ask for a change, comment or review in plain words. How Perfloop handles pull requests.