Skip to content

Validate DataRow column length before buffered read#225

Merged
SeanTAllen merged 1 commit intomainfrom
216-data-row-column-length-bounds-check
Apr 15, 2026
Merged

Validate DataRow column length before buffered read#225
SeanTAllen merged 1 commit intomainfrom
216-data-row-column-length-bounds-check

Conversation

@SeanTAllen
Copy link
Copy Markdown
Member

Closes #216.

_data_row matched the column length against -1 (NULL) and 0, but column lengths in 0x80000000..0xFFFFFFFE — invalid signed Int32 values — fell through to reader.block() without validation. The huge length errored at the buffered read and routed through the protocol-violation path, so no observable behavior changes; this makes the validation explicit at the parse site. Same flavor as #211.

Note on the test: both code paths (with and without the explicit if) error externally — reader.block() will reject any bogus length we can practically construct in a test. The test is therefore a behavioral regression test for "parser rejects malformed column length", not a strict counterfactual for the new if. Flagging in case you'd rather drop the test — I think keeping it is the right call but it's a judgment call.

The match against -1 covers the NULL marker, but column lengths in
0x80000000..0xFFFFFFFE represent invalid signed Int32 values that fell
through to reader.block() unchecked. Same flavor as #211 — make the
validation explicit at the parse site rather than relying on a
downstream read failure.

Closes #216.
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Apr 15, 2026
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Apr 15, 2026
@ponylang-main
Copy link
Copy Markdown
Contributor

Hi @SeanTAllen,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 225.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@SeanTAllen SeanTAllen merged commit b6dd472 into main Apr 15, 2026
11 checks passed
@SeanTAllen SeanTAllen deleted the 216-data-row-column-length-bounds-check branch April 15, 2026 11:28
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Apr 15, 2026
github-actions Bot pushed a commit that referenced this pull request Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_data_row column_length lacks bounds check before USize cast

2 participants