fallintoplace commented on code in PR #1097:
URL: https://github.com/apache/arrow-go/pull/1097#discussion_r3734317014
##########
arrow/ipc/file_reader.go:
##########
@@ -974,21 +977,18 @@ func (blk mappedFileBlock) NewMessage() (*Message, error)
{
metaBytes := buf[:blk.meta]
- prefix := 0
- switch binary.LittleEndian.Uint32(metaBytes) {
- case 0:
- case kIPCContToken:
- prefix = 8
- default:
- // ARROW-6314: backwards compatibility for reading old IPC
- // messages produced prior to version 0.15.0
- prefix = 4
- }
- if int(blk.meta)-prefix < 4 {
- return nil, fmt.Errorf("arrow/ipc: invalid file block metadata
length %d for prefix length %d", blk.meta, prefix)
+ prefix, err := validateFileBlockMetadata(metaBytes, blk.meta)
+ if err != nil {
+ return nil, err
}
meta = memory.NewBufferBytes(metaBytes[prefix:])
body = memory.NewBufferBytes(buf[blk.meta : int64(blk.meta)+blk.body])
- return NewMessage(meta, body), nil
+ msg := NewMessage(meta, body)
+ messageBodyLen := msg.BodyLen()
+ if messageBodyLen != blk.body {
+ msg.Release()
+ return nil, fmt.Errorf("arrow/ipc: file block body length %d
does not match message body length %d", blk.body, messageBodyLen)
+ }
+ return msg, nil
Review Comment:
They are two independent values in the IPC file: Message.bodyLength comes
from the message FlatBuffer, while Block.bodyLength comes from the file footer,
so a producer bug or malformed file can make them disagree.
There is a concrete precedent for this: apache/arrow#19596 was a JS writer
bug that produced exactly this mismatch. C++ also added this same consistency
check in apache/arrow#48845 earlier this year.
So I think this check is useful for detecting an inconsistent IPC file
rather than relying on either copy alone.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
