Fix snapshot mapping merge bug #230
Conversation
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
There was a problem hiding this comment.
Key Issues
Critical issues include missing null checks for deserialized headers, potential division by zero and overflow errors in calculations, and incorrect conditions for handling overlapping segments, all of which could lead to application crashes or incorrect data processing.
There was a problem hiding this comment.
Just to make sure I understand it correctly BuildStorageOffset corresponds to the data in the file right, which consists of only the changed blocks for that BuildId, so based on BuildStorageOffset you know where to read at that file.
I didn't check the visualizer. Thanks for tests! I think they are really helpful here. Maybe adding tests for BuildStorageOffset would make sense as well.
| @@ -88,13 +96,13 @@ func MergeMappings( | |||
| base := baseMapping[baseIdx] | |||
| diff := diffMapping[diffIdx] | |||
|
|
|||
| if base.Length == 0 { | |||
| if base.Length <= 0 { | |||
There was a problem hiding this comment.
is this just to make sure or how can it be negative?
| baseIdx++ | ||
|
|
||
| continue | ||
| } | ||
|
|
||
| if diff.Length == 0 { | ||
| if diff.Length <= 0 { |
| } | ||
|
|
||
| mappings = append(mappings, baseMapping[baseIdx:]...) | ||
| mappings = append(mappings, diffMapping[diffIdx:]...) | ||
|
|
||
| return mappings | ||
| } | ||
|
|
||
| // Format returns a string representation of the mapping as: |
There was a problem hiding this comment.
I would move this to separate file, it isn't the core logic, so it's separated
| require.True(t, Equal(m, []*BuildMap{ | ||
| { | ||
| Offset: 0, | ||
| Length: 2 * blockSize, | ||
| BuildId: ignoreID, | ||
| }, | ||
| { | ||
| Offset: 2 * blockSize, | ||
| Length: 4 * blockSize, | ||
| BuildId: baseID, | ||
| }, | ||
| { | ||
| Offset: 6 * blockSize, | ||
| Length: 2 * blockSize, | ||
| BuildId: ignoreID, | ||
| }, | ||
| })) |
There was a problem hiding this comment.
Can you do this?
| require.True(t, Equal(m, []*BuildMap{ | |
| { | |
| Offset: 0, | |
| Length: 2 * blockSize, | |
| BuildId: ignoreID, | |
| }, | |
| { | |
| Offset: 2 * blockSize, | |
| Length: 4 * blockSize, | |
| BuildId: baseID, | |
| }, | |
| { | |
| Offset: 6 * blockSize, | |
| Length: 2 * blockSize, | |
| BuildId: ignoreID, | |
| }, | |
| })) | |
| require.True(t, Equal(m, simpleBase)) |
There is a bug that sometimes happens during the sandbox pause. This PR fixed it. It will also include tests for the merged mapping.
✨
Description by Callstackai
This PR fixes a bug related to snapshot mapping merging during sandbox pause and includes tests for the merged mapping functionality.
Diagrams of code changes
sequenceDiagram participant Client participant HeaderMerger participant Storage participant Validator Client->>Storage: Load Base Header Storage-->>Client: Base Header Data Client->>Storage: Load Diff Header Storage-->>Client: Diff Header Data Client->>HeaderMerger: MergeMappings(baseHeader, diffHeader) activate HeaderMerger HeaderMerger->>HeaderMerger: Process Mapping Overlaps HeaderMerger->>HeaderMerger: Handle Base/Diff Positions HeaderMerger-->>Client: Merged Header deactivate HeaderMerger Client->>Validator: ValidateMappings(mergedHeader) activate Validator Validator->>Validator: Check Contiguous Blocks Validator->>Validator: Verify Block Sizes Validator->>Validator: Validate Total Size Validator-->>Client: Validation Result deactivate Validator opt Visualization Enabled Client->>Client: Visualize Header Blocks endFiles Changed