GH-43833: [Integration] validate that arrow IPC files include a valid embedded IPC stream #43834
Conversation
This file contains 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
github-actions
bot
added
Component: C++
awaiting committer review
pitrou
reviewed
Aug 29, 2024
| ARROW_ASSIGN_OR_RAISE(auto footer_cookie, arrow_file->ReadAt(file_size - 10, 10)); | ||
| auto footer_size = | ||
| bit_util::FromLittleEndian(util::SafeLoadAs<int32_t>(footer_cookie->data())); | ||
| int64_t footer_offset = 8 + file_size - footer_size - 10; |
There was a problem hiding this comment.
The 8 + is a bit cryptic. I realize it accounts for the 8-byte padded magic at the start of file, but a comment would make this easier to understand.
Or perhaps you can change the checks to something like:
// The footer is followed by the 10-byte footer length + magic
const int64_t footer_offset = file_size - footer_size - 10;
// Make sure the stream ends before the footer
ARROW_ASSIGN_OR_RAISE(const int64_t stream_size, stream->Tell());
const int64_t stream_end_offset = stream_size + 8;
if (stream_end_offset > footer_offset) {
pitrou
reviewed
Aug 29, 2024
| } | ||
| ARROW_ASSIGN_OR_RAISE(int64_t stream_size, stream->Tell()); | ||
|
|
||
| if (footer_offset <= stream_size) { |
There was a problem hiding this comment.
Hmm, why not footer_offset < stream_size? It should be ok if the stream ends just before the footer.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Rationale for this change
IPC files should contain a valid IPC stream with EOS. Some arrow producers don't guarantee that the EOS is written. However there might be readers depending on simply reading the embedded IPC stream (and depending on the EOS to avoid reading the Footer as a corrupted message) so we should validate stream reading from IPC files as well as file reading.
What changes are included in this PR?
arrow-json-integration-test will now also try to read a file as a stream, which will fail if the file doesn't include an EOS or otherwise isn't a valid stream.
Are these changes tested?
yes
Are there any user-facing changes?
no