Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

pitrou commented on PR #49771:
URL: https://github.com/apache/arrow/pull/49771#issuecomment-4720125462

   I'm thinking of another problem: this will detect the IPC file marker for 
every metadata message, but we actually only want to do that at the start of 
the stream. `State::INITIAL` would not be enough to detect that, because we go 
back to `INITIAL` after each successfully decoded message.
   
   Perhaps `DecodeMessage` is not the right place for this and this should be 
done at a higher level in the call tree?
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

pitrou commented on code in PR #49771:
URL: https://github.com/apache/arrow/pull/49771#discussion_r3421700396


##
cpp/src/arrow/ipc/message.cc:
##
@@ -563,7 +563,25 @@ Status DecodeMessage(MessageDecoder* decoder, 
io::InputStream* file) {
   }
 
   auto metadata_length = decoder->next_required_size();
+
+  // "ARRO" (first 4 bytes of kArrowMagicBytes) as little-endian int32.
+  constexpr int32_t kArrowMagicPrefix = 0x4F525241;
+
+  // Did we misinterpret the metadata as a length?
+  if (metadata_length == kArrowMagicPrefix) {
+constexpr std::string_view kRemainingMagic =
+internal::kArrowMagicBytes.substr(sizeof(int32_t));
+ARROW_ASSIGN_OR_RAISE(auto peek, file->Read(kRemainingMagic.size()));
+if (peek->size() >= static_cast(kRemainingMagic.size()) &&
+std::string_view(reinterpret_cast(peek->data()),
+ kRemainingMagic.size()) == kRemainingMagic) {

Review Comment:
   Letting it fall through means a mythical user with a valid 0x4F525241 bytes 
metadata block will get a regression with a weird message decoding the 
Flatbuffers metadata. I'd rather an explicit message.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

RobertLD commented on code in PR #49771:
URL: https://github.com/apache/arrow/pull/49771#discussion_r3416218431


##
cpp/src/arrow/ipc/message.cc:
##
@@ -563,7 +563,25 @@ Status DecodeMessage(MessageDecoder* decoder, 
io::InputStream* file) {
   }
 
   auto metadata_length = decoder->next_required_size();
+
+  // "ARRO" (first 4 bytes of kArrowMagicBytes) as little-endian int32.
+  constexpr int32_t kArrowMagicPrefix = 0x4F525241;
+
+  // Did we misinterpret the metadata as a length?
+  if (metadata_length == kArrowMagicPrefix) {
+constexpr std::string_view kRemainingMagic =
+internal::kArrowMagicBytes.substr(sizeof(int32_t));
+ARROW_ASSIGN_OR_RAISE(auto peek, file->Read(kRemainingMagic.size()));
+if (peek->size() >= static_cast(kRemainingMagic.size()) &&
+std::string_view(reinterpret_cast(peek->data()),
+ kRemainingMagic.size()) == kRemainingMagic) {

Review Comment:
   @pitrou Okay I see what you're saying and this was an oversight on my part. 
My idea is to continue to let it fall through and add a separate check 
immediately below it to reject metadata over 1GB. I think this is the best 
solution but it does change the behavior of the library (and I'm concerned 
there may be a single user somewhere who has 10Gb of metadata) and it's a bit 
of stretch scope wise considering this ticket is about error messages.
   
   What are your thoughts? I'm thinking I just go forward with the above (or 
alternatively, just throw a failure with a different message on specifically 
0x4F525241 even if it's not IPC)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

RobertLD commented on PR #49771:
URL: https://github.com/apache/arrow/pull/49771#issuecomment-4545598857

   I will address this last comment shortly, but am on vacation. Just pinging 
this so it doesn't get closed as stale 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

pitrou commented on code in PR #49771:
URL: https://github.com/apache/arrow/pull/49771#discussion_r3234304108


##
cpp/src/arrow/ipc/message.cc:
##
@@ -563,7 +563,25 @@ Status DecodeMessage(MessageDecoder* decoder, 
io::InputStream* file) {
   }
 
   auto metadata_length = decoder->next_required_size();
+
+  // "ARRO" (first 4 bytes of kArrowMagicBytes) as little-endian int32.
+  constexpr int32_t kArrowMagicPrefix = 0x4F525241;
+
+  // Did we misinterpret the metadata as a length?
+  if (metadata_length == kArrowMagicPrefix) {
+constexpr std::string_view kRemainingMagic =
+internal::kArrowMagicBytes.substr(sizeof(int32_t));
+ARROW_ASSIGN_OR_RAISE(auto peek, file->Read(kRemainingMagic.size()));
+if (peek->size() >= static_cast(kRemainingMagic.size()) &&
+std::string_view(reinterpret_cast(peek->data()),
+ kRemainingMagic.size()) == kRemainingMagic) {

Review Comment:
   What happens if this check fails? Are we assuming that the file is a valid 
IPC stream that happens to have a metadata size exactly 0x4F525241 bytes?
   
   In any case, we have read `kRemainingMagic` bytes that are discarded below, 
so we'll be out of sync anyway.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

RobertLD commented on PR #49771:
URL: https://github.com/apache/arrow/pull/49771#issuecomment-4432451158

   > Also can you make the PR description a bit more specific @RobertLD ?
   
   Addressed this and other PR suggestions with my latest commit 
   
[d01cb5d](https://github.com/apache/arrow/pull/49771/commits/d01cb5d5b73abe0a90472f6890cd65dc1e13e91c)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

RobertLD commented on code in PR #49771:
URL: https://github.com/apache/arrow/pull/49771#discussion_r3227948599


##
cpp/src/arrow/ipc/read_write_test.cc:
##
@@ -2265,6 +2265,52 @@ TEST(TestRecordBatchStreamReader, MalformedInput) {
   ASSERT_RAISES(Invalid, RecordBatchStreamReader::Open(&garbage_reader));
 }
 
+TEST(TestRecordBatchStreamReader, OpenFileFormatSuggestsFileReader) {
+  std::shared_ptr batch;
+  ASSERT_OK(MakeIntRecordBatch(&batch));
+
+  FileWriterHelper helper;
+  ASSERT_OK(helper.Init(batch->schema(), IpcWriteOptions::Defaults()));
+  ASSERT_OK(helper.WriteBatch(batch));
+  ASSERT_OK(helper.Finish());
+
+  io::BufferReader reader(helper.buffer_);
+  // Check we mention using the file_reader when we detect file format
+  EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("file reader"),
+  RecordBatchStreamReader::Open(&reader));
+}
+
+TEST(TestRecordBatchStreamReader, CorruptDataDoesNotSuggestFileReader) {
+  // Continuation marker + metadata_length = 100, then 8 bytes of non-magic 
data.
+  const std::string corrupt(
+  "\xff\xff\xff\xff"
+  "\x64\x00\x00\x00"
+  "ABABABAB",
+  16);
+  auto buffer = std::make_shared(corrupt);
+  io::BufferReader reader(buffer);
+  // Validate that we don't suggest file reader when file is just corrupt
+  EXPECT_RAISES_WITH_MESSAGE_THAT(
+  Invalid, ::testing::Not(::testing::HasSubstr("file reader")),
+  RecordBatchStreamReader::Open(&reader));
+}
+
+TEST(TestRecordBatchFileReader, OpenStreamFormatSuggestsStreamReader) {
+  std::shared_ptr batch;
+  ASSERT_OK(MakeIntRecordBatch(&batch));
+
+  StreamWriterHelper helper;
+  ASSERT_OK(helper.Init(batch->schema(), IpcWriteOptions::Defaults()));
+  ASSERT_OK(helper.WriteBatch(batch));
+  ASSERT_OK(helper.Finish());
+
+  auto buf_reader = std::make_shared(helper.buffer_);
+  // Check we mention using the stream_reader when we detect stream format
+  EXPECT_RAISES_WITH_MESSAGE_THAT(
+  Invalid, ::testing::HasSubstr("stream reader"),

Review Comment:
   ditto



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

RobertLD commented on code in PR #49771:
URL: https://github.com/apache/arrow/pull/49771#discussion_r3227946787


##
cpp/src/arrow/ipc/message.cc:
##
@@ -565,6 +565,17 @@ Status DecodeMessage(MessageDecoder* decoder, 
io::InputStream* file) {
   auto metadata_length = decoder->next_required_size();
   ARROW_ASSIGN_OR_RAISE(auto metadata, file->Read(metadata_length));
   if (metadata->size() != metadata_length) {
+// The first sizeof(int32_t) bytes of the Arrow file magic ("ARRO") may 
have been
+// misread as metadata_length. Check if the remaining bytes complete the 
magic.

Review Comment:
   I moved the check to happen first, and avoid the large read if possible



##
cpp/src/arrow/ipc/read_write_test.cc:
##
@@ -2265,6 +2265,52 @@ TEST(TestRecordBatchStreamReader, MalformedInput) {
   ASSERT_RAISES(Invalid, RecordBatchStreamReader::Open(&garbage_reader));
 }
 
+TEST(TestRecordBatchStreamReader, OpenFileFormatSuggestsFileReader) {
+  std::shared_ptr batch;
+  ASSERT_OK(MakeIntRecordBatch(&batch));
+
+  FileWriterHelper helper;
+  ASSERT_OK(helper.Init(batch->schema(), IpcWriteOptions::Defaults()));
+  ASSERT_OK(helper.WriteBatch(batch));
+  ASSERT_OK(helper.Finish());
+
+  io::BufferReader reader(helper.buffer_);
+  // Check we mention using the file_reader when we detect file format
+  EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("file reader"),

Review Comment:
   addressed



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

paleolimbot commented on code in PR #49771:
URL: https://github.com/apache/arrow/pull/49771#discussion_r3203177106


##
cpp/src/arrow/ipc/message.cc:
##
@@ -565,6 +565,17 @@ Status DecodeMessage(MessageDecoder* decoder, 
io::InputStream* file) {
   auto metadata_length = decoder->next_required_size();
   ARROW_ASSIGN_OR_RAISE(auto metadata, file->Read(metadata_length));
   if (metadata->size() != metadata_length) {
+// The first sizeof(int32_t) bytes of the Arrow file magic ("ARRO") may 
have been
+// misread as metadata_length. Check if the remaining bytes complete the 
magic.

Review Comment:
   In nanoarrow we check the first few bytes for the magic string and skip them 
(then attempt to read the rest of the input as an IPC stream). We've never run 
into a complaint about this not working but I'm not sure how widespread the 
usage is (we could add an option to turn it off or improve the error that 
occurs if we run into one). I think 1330795073 bytes of metadata would never 
reasonably occur on purpose.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

RobertLD commented on PR #49771:
URL: https://github.com/apache/arrow/pull/49771#issuecomment-4398896437

   I’ll circle back in a day or two and apply these suggestions. Thanks for
   the feedback
   
   On Thu, May 7, 2026 at 09:45 David Li ***@***.***> wrote:
   
   > ***@***. commented on this pull request.
   > --
   >
   > In cpp/src/arrow/ipc/message.cc
   > :
   >
   > > @@ -565,6 +565,17 @@ Status DecodeMessage(MessageDecoder* decoder, 
io::InputStream* file) {
   >auto metadata_length = decoder->next_required_size();
   >ARROW_ASSIGN_OR_RAISE(auto metadata, file->Read(metadata_length));
   >if (metadata->size() != metadata_length) {
   > +// The first sizeof(int32_t) bytes of the Arrow file magic ("ARRO") 
may have been
   > +// misread as metadata_length. Check if the remaining bytes complete 
the magic.
   >
   > Yeah, a gigabyte of *metadata* raises flags anyways
   >
   > —
   > Reply to this email directly, view it on GitHub
   > , or
   > unsubscribe
   > 

   > .
   > You are receiving this because you were mentioned.Message ID:
   > ***@***.***>
   >
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

lidavidm commented on code in PR #49771:
URL: https://github.com/apache/arrow/pull/49771#discussion_r3201879954


##
cpp/src/arrow/ipc/message.cc:
##
@@ -565,6 +565,17 @@ Status DecodeMessage(MessageDecoder* decoder, 
io::InputStream* file) {
   auto metadata_length = decoder->next_required_size();
   ARROW_ASSIGN_OR_RAISE(auto metadata, file->Read(metadata_length));
   if (metadata->size() != metadata_length) {
+// The first sizeof(int32_t) bytes of the Arrow file magic ("ARRO") may 
have been
+// misread as metadata_length. Check if the remaining bytes complete the 
magic.

Review Comment:
   Yeah, a gigabyte of _metadata_ raises flags anyways



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] GH-33823: [C++][IPC] Improve error messages when opening files that are the wrong format [arrow]

[via GitHub]

pitrou commented on PR #49771:
URL: https://github.com/apache/arrow/pull/49771#issuecomment-4397505982

   Also can you make the PR description a bit more specific @RobertLD ?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org