Feature/serialization - #62
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6836752645
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!samples_fit(select1_sample_begin_, select1_sample_count_) || | ||
| !samples_fit(select0_sample_begin_, select0_sample_count_) || | ||
| (builds_select1(select_support_) != (select1_sample_count_ != 0)) || | ||
| (builds_select0(select_support_) != (select0_sample_count_ != 0))) { |
There was a problem hiding this comment.
When a serialized select-sample word is corrupted to an out-of-range superblock index, these checks still accept the artifact because they validate only the sample array's offsets and counts. A subsequent select() uses that unchecked value in find_superblock(), causing an out-of-bounds metadata read; validate every stored sample against the available superblock range before returning the deserialized index.
Useful? React with 👍 / 👎.
| if (!valid_node_index(node.parent) || | ||
| !valid_node_index(node.left_child) || | ||
| !valid_node_index(node.right_child) || | ||
| node.data.size() > node.bit_vector_data.size_bits()) { |
There was a problem hiding this comment.
For a malformed artifact whose child or parent points to the same node, valid_node_index() succeeds because it checks only bounds. The deserialized object is then unsafe to query: rank() follows child links until npos and select() follows parent links, so such an artifact hangs indefinitely. Validate that the links form an acyclic rooted tree with consistent parent-child relationships before accepting it.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
==========================================
+ Coverage 90.31% 90.53% +0.21%
==========================================
Files 41 45 +4
Lines 9109 11475 +2366
Branches 1466 1721 +255
==========================================
+ Hits 8227 10389 +2162
- Misses 456 535 +79
- Partials 426 551 +125
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a360c58999
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (required_words > std::numeric_limits<std::size_t>::max() / kWordSize) { | ||
| throw std::length_error("RankSelectSupport padded size is too large"); | ||
| } | ||
| if (padded_size_ != required_words * kWordSize || max_rank_ > num_bits_) { |
There was a problem hiding this comment.
When a serialized artifact changes max_rank_ to any other value not exceeding num_bits_, this validation accepts it even though rank(num_bits_), rank0(), and select bounds rely directly on that field. For example, metadata for a 64-one source is accepted with max_rank_ = 1, after which rank(64) returns 1 while shorter-prefix ranks still return their original values; validate the exact one count against the logical source bits before accepting the metadata.
Useful? React with 👍 / 👎.
| for (const node_index_t leaf : result.leaves_) { | ||
| if (!valid_node_index(leaf)) { | ||
| throw std::invalid_argument("Invalid serialized wavelet-tree leaf"); |
There was a problem hiding this comment.
For a serialized nonempty tree, changing a leaf entry to npos or another in-range node passes this bounds-only check. select() then starts from that forged entry and either returns rank - 1 immediately or follows the wrong parent chain, silently producing incorrect positions; verify that every permuted symbol's leaf is the terminal node reached by following that symbol through the validated tree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29c247118d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!valid_node_index(node.parent) || | ||
| !valid_node_index(node.left_child) || | ||
| !valid_node_index(node.right_child) || | ||
| node.data.size() > node.bit_vector_data.size_bits() || |
There was a problem hiding this comment.
When a serialized node has a child whose rank-vector length is smaller than the number of bits the parent routes to that child, this local capacity check and the later topology validation still accept it. get_segment() then recurses using the parent-derived range and directly indexes the child's bit_vector_data through that larger range, reading outside the child's span; verify that each present child's logical length exactly equals the corresponding zero/one count in its parent.
Useful? React with 👍 / 👎.
| if (position < begin || position >= end) { | ||
| throw std::invalid_argument( | ||
| "Invalid serialized RMQ sparse-table candidate"); |
There was a problem hiding this comment.
When a valid artifact is paired with a different value array of the same length, validation checks only that each sparse candidate lies within its covered range and that the stored BP sequence is balanced, not that either represents values. For example, an artifact built over {0,1,2,3} is accepted over {3,2,1,0} and arg_min(0,4) returns 0 instead of 3; reconstruct or otherwise validate the Cartesian encoding and candidate minima against the supplied values before accepting it.
Useful? React with 👍 / 👎.
| if (segment_size_bits[node] != expected_size || | ||
| std::abs(total) > static_cast<std::int64_t>(expected_size) || | ||
| ((static_cast<std::int64_t>(expected_size) + total) & 1) != 0 || | ||
| minimum > total || maximum < total || | ||
| minimum < -static_cast<std::int64_t>(expected_size) || | ||
| maximum > static_cast<std::int64_t>(expected_size) || | ||
| node_min_count[node] == 0 || node_min_count[node] > expected_size || | ||
| node_pattern10_count[node] >= expected_size || | ||
| node_first_bit[node] > 1 || node_last_bit[node] > 1) { |
There was a problem hiding this comment.
When an otherwise valid artifact is deserialized over a different same-length bit sequence, these checks establish only that each leaf's statistics are plausible and internally composable, not that they describe the supplied words. An artifact built for 128 one-bits with 8-bit leaves is accepted over 128 zero-bits, after which rank1(65) returns 64 instead of 0; recompute each leaf's excess/min/max/count/edge statistics from bits before accepting the metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1e6a0032a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (validation == DeserializationValidation::kFull) { | ||
| validate_full_source_metadata(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When the default quick mode reads corrupted super-block or basic-block rank values, it validates their storage sizes but not the prefix invariants required by select(). For example, changing the first super-block prefix from 0 to 1 in an artifact over the one-word source {1} is accepted; select(1) then makes find_superblock() return size_t(-1) and reads before the metadata allocation. Quick validation should at least check that prefix arrays begin at zero and remain monotonic and bounded so queries are memory-safe, while leaving exact source authentication to full mode.
Useful? React with 👍 / 👎.
| if (root_ == npos) { | ||
| if (validation == DeserializationValidation::kFull && | ||
| std::ranges::any_of(leaves_, | ||
| [](node_index_t leaf) { return leaf != npos; })) { | ||
| throw std::invalid_argument( | ||
| "Invalid serialized empty wavelet-tree leaves"); | ||
| } | ||
| return; |
There was a problem hiding this comment.
When an artifact for an empty one-symbol alphabet has its serialized data_size_ changed from 0 to a positive value, both quick and full validation take this early return and accept root_ == npos with no nodes. The restored object's size() then advertises valid positions, but get_segment(0, 1) calls copy_segment_content(npos, ...) and dereferences nodes_[npos]; require data_size_ == 0 whenever the root is absent.
Useful? React with 👍 / 👎.
1 participant
Uh oh!
There was an error while loading. Please reload this page.