fix(array): don't abort on a NUL byte in a field name - #8664
Closed
miniex wants to merge 1 commit into
Closed
Conversation
an interior NUL in a struct field name aborted the process when the schema was exported over the arrow c ffi: arrow copies the name into a `CString` in a non-unwinding context, so the failure escalated to `SIGABRT`. `to_arrow_schema` and `ArrowSession::to_arrow_field` now reject such names, returning a `VortexError` instead. Closes vortex-data#8652 Signed-off-by: Han Damin <miniex@daminstudio.net>
Contributor
|
This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days |
Comment on lines
+314
to
+319
| pub(crate) fn validate_arrow_field_name(name: &str) -> VortexResult<()> { | ||
| if name.contains('\0') { | ||
| vortex_bail!("field name {name:?} cannot be exported to Arrow: contains a NUL byte"); | ||
| } | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
Does arrow use nul str or str + len
Contributor
There was a problem hiding this comment.
I made this fix in arrow-rs apache/arrow-rs#10328. Arrow supports names that are not valid c strings unfortunately
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
3 participants
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.
Summary
A struct field name with an interior NUL byte aborted the whole process (
SIGABRT) when the schema was exported across the Arrow C Data FFI: arrow-rs copies the name into aCStringin a non-unwinding context, soCString::newfailing on the NUL escalated to an abort instead of an error (#8652).to_arrow_schema/to_data_type_naiveandArrowSession::to_arrow_fieldnow reject a NUL byte in a field name up front, so a crafted or mislabeled file surfaces a recoverableVortexErrorinstead of taking down the reader.This rejects only NUL, the byte that actually aborts (other control characters are valid in a
CString), and guards the Arrow export boundary rather than the writer, so it also covers aDTypefrom a non-Vortex producer.Closes: #8652
Testing
Added tests that
to_arrow_schema(top-level and nested struct) andArrowSession::to_arrow_fieldreject a NUL field name.cargo nextest run -p vortex-arraypasses;fmt --all+clippy --all-targets --all-featuresclean.I'm Korean, so sorry if any wording reads a little awkward.