Conversation
When TableReader_ReadRow failed, for example for an external reference that isn't available, ReferenceObj_Read still read the SEQ_LEN cell of the row: a stale value if a row of another reference had been read before, otherwise a NULL dereference. The loop ends on a failed read either way, so the check now runs only after a successful read.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for maintaining ncbi-vdb. This fixes the segfault described in #282:
ReferenceObj_Read()reads the SEQ_LEN cell of a row even whenTableReader_ReadRow()failed, which dereferences NULL when no row has been read through the reference list yet.Change
The SEQ_LEN check that ends the loop at a reference's last row now runs inside
if ( rc == 0 ), after the row has been read. On a failed read the loop ends anyway (while ( rc == 0 && ... )), so behaviour is otherwise unchanged.Testing
On macOS 26.6.1 arm64, Apple clang, against a debug build of
engineering, with the harness from the issue on SRR390728 without its references and remote access disabled:reference.c:1030.rc=0x9be50398with nothing written; references whose data are present read the same 5,000 bases as before.No regression test is included, since it needs an aligned archive whose external references are unavailable. I'm happy to add one if you can suggest where.
I dedicate this change to the public domain, consistent with the project's Public Domain Notice. Thanks for considering it; I'm glad to rework it in whatever way you prefer.