zeroshade commented on code in PR #1097:
URL: https://github.com/apache/arrow-go/pull/1097#discussion_r3730440907
##########
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:
how can this actually happen?
##########
arrow/ipc/metadata.go:
##########
@@ -75,6 +75,38 @@ func (blk fileBlock) Offset() int64 { return blk.offset }
func (blk fileBlock) Meta() int32 { return blk.meta }
func (blk fileBlock) Body() int64 { return blk.body }
+func validateFileBlockMetadata(buf []byte, meta int32) (int, error) {
+ if len(buf) < 4 {
+ return 0, fmt.Errorf("arrow/ipc: file block metadata is too
short: %d", len(buf))
+ }
+
+ var (
+ prefix int
+ length uint32
+ )
+ switch binary.LittleEndian.Uint32(buf) {
+ case 0:
+ return 0, errors.New("arrow/ipc: unexpected end-of-stream
marker in file block")
+ case kIPCContToken:
+ prefix = 8
+ if len(buf) < prefix {
+ return 0, fmt.Errorf("arrow/ipc: file block metadata is
too short for prefix length %d", prefix)
+ }
+ length = binary.LittleEndian.Uint32(buf[4:])
+ default:
+ prefix = 4
+ length = binary.LittleEndian.Uint32(buf)
+ }
Review Comment:
these can be factored out
```suggestion
default:
prefix = 4
}
length = binary.LittleEndian.Uint32(buf[prefix-4:])
```
##########
arrow/ipc/metadata.go:
##########
@@ -75,6 +75,38 @@ func (blk fileBlock) Offset() int64 { return blk.offset }
func (blk fileBlock) Meta() int32 { return blk.meta }
func (blk fileBlock) Body() int64 { return blk.body }
+func validateFileBlockMetadata(buf []byte, meta int32) (int, error) {
+ if len(buf) < 4 {
+ return 0, fmt.Errorf("arrow/ipc: file block metadata is too
short: %d", len(buf))
+ }
+
+ var (
+ prefix int
+ length uint32
+ )
+ switch binary.LittleEndian.Uint32(buf) {
+ case 0:
+ return 0, errors.New("arrow/ipc: unexpected end-of-stream
marker in file block")
+ case kIPCContToken:
+ prefix = 8
+ if len(buf) < prefix {
+ return 0, fmt.Errorf("arrow/ipc: file block metadata is
too short for prefix length %d", prefix)
+ }
+ length = binary.LittleEndian.Uint32(buf[4:])
+ default:
+ prefix = 4
Review Comment:
you dropped the comment explaining that this is for reading old IPC messages
from Arrow format version <= 0.15.0, add the comment back please
##########
arrow/ipc/file_reader.go:
##########
@@ -75,6 +75,9 @@ func validateFileBlock(offset int64, meta int32, body,
fileSize, maxMetadataSize
if body < 0 {
return fmt.Errorf("arrow/ipc: invalid file block body length
%d", body)
}
+ if body%8 != 0 {
+ return fmt.Errorf("arrow/ipc: file block body length %d is not
a multiple of 8", body)
+ }
Review Comment:
while we always make sure we output a body with a multiple of 8, let's be
lenient on reads and not reject these
##########
arrow/ipc/metadata.go:
##########
@@ -131,7 +154,13 @@ func (blk fileBlock) NewMessage() (*Message, error) {
return nil, fmt.Errorf("arrow/ipc: could not read message body:
%w", err)
}
- 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:
same question, how does this actually happen?
--
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]
