laskoviymishka commented on code in PR #1678:
URL: https://github.com/apache/iceberg-go/pull/1678#discussion_r3862374695
##########
puffin/puffin_reader.go:
##########
@@ -74,6 +79,17 @@ func WithMaxBlobSize(size int64) ReaderOption {
}
}
+// WithMaxFooterSize sets the maximum decompressed size allowed for compressed
Review Comment:
The limit here only binds compressed footers. An uncompressed footer ignores
`WithMaxFooterSize` entirely, and a non-positive value that errors for
compressed reads is silently a no-op for uncompressed ones. That's a little
surprising from the name: someone setting `WithMaxFooterSize(N)` on untrusted
input gets no protection when the footer happens to be uncompressed.
I'd either enforce it on the uncompressed path too, or rename to
`WithMaxCompressedFooterSize` / `DefaultMaxCompressedFooterSize` so the scope
is obvious from the call site. Either's fine, I just don't want the asymmetry
to be a surprise. wdyt?
##########
puffin/puffin_reader.go:
##########
@@ -230,11 +441,65 @@ func (r *Reader) readFooter() error {
payloadReader = io.NewSectionReader(r.r, footerStart+MagicSize,
payloadSize)
}
- decoder := json.NewDecoder(payloadReader)
+ var footerReader io.Reader = payloadReader
+ var compressedFooter *countingReader
+ var compressedContentSize uint64
+ var limitedFooter *io.LimitedReader
+ if flags&FooterFlagCompressed != 0 {
Review Comment:
In round 1 I said the ecosystem note could wait for the writer side. I'll
walk that back a touch. Java's `PuffinFormat.decompress()` still throws on the
LZ4 branch, and PyIceberg and rust reject compressed footers too, so a file we
read here won't reopen in any of them today.
A one-line comment right at this branch noting that would be a nice
breadcrumb for whoever adds writer support, so they know not to flip
`FooterFlagCompressed` on until the others catch up. Still not a blocker, just
cheap insurance.
##########
puffin/puffin_test.go:
##########
@@ -671,6 +730,204 @@ func TestReaderRejectsTrailingFooterData(t *testing.T) {
}
}
+func TestReaderReadsLZ4CompressedFooter(t *testing.T) {
+ data := fileWithCompressedFooterPayload(t,
[]byte(`{"blobs":[],"properties":{"source":"lz4"}}`))
+
+ r, err := puffin.NewReader(bytes.NewReader(data))
+ require.NoError(t, err)
+ assert.Equal(t, "lz4", r.Properties()["source"])
+ assert.Empty(t, r.Blobs())
+}
+
+func TestReaderReadsExternalLZ4CompressedFooter(t *testing.T) {
+ data := readFixture(t, "compressed-footer-lz4-cli.puffin")
+
+ r, err := puffin.NewReader(bytes.NewReader(data))
+ require.NoError(t, err)
+ assert.Equal(t, "lz4-cli", r.Properties()["source"])
+ assert.Empty(t, r.Blobs())
+}
+
+func TestReaderReadsLZ4CompressedFooterWithBlockChecksums(t *testing.T) {
+ payload := []byte(`{"blobs":[]}`)
+ data := fileWithCompressedFooterPayloadWithOptions(
+ t,
+ payload,
+ lz4.SizeOption(uint64(len(payload))),
+ lz4.BlockChecksumOption(true),
+ )
+
+ _, err := puffin.NewReader(bytes.NewReader(data))
+ require.NoError(t, err)
+}
+
+func TestReaderPreservesLZ4ChecksumError(t *testing.T) {
+ payload := []byte(`{"blobs":[]}`)
+ data := fileWithCompressedFooterPayloadWithOptions(
+ t,
+ payload,
+ lz4.SizeOption(uint64(len(payload))),
+ lz4.ChecksumOption(true),
+ )
+ data[len(data)-13] ^= 0xff
+
+ _, err := puffin.NewReader(bytes.NewReader(data))
+ require.ErrorContains(t, err, "invalid frame checksum")
+}
+
+func TestReaderPreservesLZ4BlockChecksumError(t *testing.T) {
+ payload := []byte(`{"blobs":[]}`)
+ frame := compressedLZ4Frame(
+ t,
+ payload,
+ lz4.SizeOption(uint64(len(payload))),
+ lz4.BlockChecksumOption(true),
+ lz4.ChecksumOption(false),
+ )
+ const lz4FrameHeaderSizeWithContentExternal = 15
Review Comment:
These two locals shadow the real package consts, and `blockChecksumOffset`
is computed straight from them. If `lz4FrameHeaderSizeWithContent` or the
block-size mask ever changes, this test keeps using the stale copies, mutates
the wrong byte, and quietly stops testing the block-checksum path (or panics).
Since `puffin_reader_internal_test.go` is already `package puffin`, I'd move
this case there and reference the real consts.
Same theme, not worth its own thread: the `data[12]` and
`data[len(data)-13]` offsets in the flag and checksum tests are hand-derived
from the header layout and would drift the same way. A named const or a short
comment would anchor them.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
