Repository navigation
pxf: DatasetReader reads under the caller's bytes and depth limits (#160) - #162
Merged
Merged
Conversation
…#160) DatasetReader::Next passed only the nesting depth to ParseDatasetRow, which read every row under kMaxBytesLiteralLength. A caller who lowered max_bytes_literal_length still got a row with a b"..." cell past it, and one who raised it had that cell refused. protowire-go reads rows under the caller's limit (dataset_stream.go: maxBytesLiteral: tr.lim.maxBytesLiteral). ParseDatasetRow now takes the limit, and the reader passes opts_.max_bytes_literal_length, with 0 meaning the default. The new test reads a 192-byte cell at limits 191 (refused, with the lexer's text), 192 and 0 (both accepted). With the reader's limit forced back to the default, the test fails at 191.
ReadHeader called Parse(header) with default ParseOptions, so a @Header block or the @dataset line was read under kMaxNestingDepth and kMaxBytesLiteralLength whatever the options said. protowire-go parses "the header and every row ... under its MaxNestingDepth and MaxBytesLiteralLength" (NewDatasetReader's doc). The new test checks both limits at Create. Measured against protowire-go 903b4a7 on the same inputs: a 192-byte b"..." in @Header is refused at 191 with 1:21: bytes literal decodes to more than MaxBytesLiteralLength=191 bytes, and a header four blocks deep is refused at depth 3 with 1:25: nesting depth exceeds MaxNestingDepth=3 and accepted at 4. This port now gives the same text and positions. With the header fix reverted, the test fails.
This file contains hidden or 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
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.
Fixes #160.
DatasetReadernow reads under the caller'smax_bytes_literal_lengthandmax_nesting_depth, as protowire-go does. Before, a caller who lowered the bytes limit still got a row whoseb"…"cell was past it, and a caller who raised it had the cell refused.Changes
ParseDatasetRowtakes the bytes-literal limit, andDatasetReader::Nextpassesopts_.max_bytes_literal_length, with0meaning the default. Only the nesting depth reached it before.ReadHeadercalledParse(header)with default options, so a@headerblock and the@datasetline ignored both limits. It now passes the caller's depth and bytes limits. Go'sNewDatasetReaderdoc says "the header and every row are parsed under its MaxNestingDepth and MaxBytesLiteralLength".Evidence
Same inputs through protowire-go 903b4a7 (
UnmarshalOptions{...}.NewDatasetReader) and this branch:b"…"2:3: bytes literal decodes to more than MaxBytesLiteralLength=191 bytes1:3:(see below)@headerwith a 192-byteb"…"1:21: bytes literal decodes to more than MaxBytesLiteralLength=191 bytes@headerfour blocks deep1:25: nesting depth exceeds MaxNestingDepth=3HardeningDataset.RowBytesLiteralUnderTheCallersLimitandHardeningDataset.HeaderUnderTheCallersLimits. Each fails with its half of the fix reverted (checked by hand for both).Not in this PR
A row error's line counts from the start of the row, where Go counts from the start of the stream (
1:3:here vs Go's2:3:). That predates this change and has its own mechanism, so it's filed as #161.