Repository navigation
feat: Validate UTF-8 at NANOARROW_VALIDATION_LEVEL_FULL - #981
rustyconover wants to merge 2 commits into
Conversation
NANOARROW_VALIDATION_LEVEL_FULL is meant for untrusted input, but it did not check that string values are valid UTF-8, which Arrow C++'s ValidateFull() does. FULL validation now checks that every non-null string and large_string value is valid UTF-8 (RFC 3629: no overlong encodings, surrogates, code points above U+10FFFF, stray continuation bytes or truncated sequences). As in Arrow C++, values are checked one at a time (a code point split across two values is invalid) and null values are not checked. ASCII is skipped eight bytes at a time, and the referenced bytes are first checked as one contiguous range plus a code point boundary check at each offset, so that short values are not validated one call at a time. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #981 +/- ##
==========================================
+ Coverage 78.30% 78.38% +0.08%
==========================================
Files 106 106
Lines 16919 17127 +208
Branches 2016 2018 +2
==========================================
+ Hits 13248 13425 +177
- Misses 2429 2459 +30
- Partials 1242 1243 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Thank you for taking this on!
I see you have a PR up for string view as well, so if it's easier to handle views together you can skip the string view comments here.
| for (int64_t i = 0; i < array_view->length; i++) { | ||
| if (ArrowArrayViewIsNull(array_view, i)) { | ||
| continue; | ||
| } | ||
|
|
||
| int64_t start = ArrowArrayViewGetOffsetUnsafe(array_view, array_view->offset + i); | ||
| int64_t end = ArrowArrayViewGetOffsetUnsafe(array_view, array_view->offset + i + 1); | ||
| if (end > start) { | ||
| NANOARROW_RETURN_NOT_OK(ArrowArrayViewValidateUtf8Element( | ||
| array_view, i, data + start, end - start, error)); | ||
| } | ||
| } |
There was a problem hiding this comment.
ArrowArrayViewGetStringUnsafe() would handle the view case as well, but might be slower. If I were trying to make the iteration go as fast as possible here I would write a separate loop for the string, large string, and string view cases (but worth benchmarking).
| int ok = ArrowUtf8ValidPrefix(data + first, last - first) == last - first; | ||
| for (int64_t i = 1; ok && i < array_view->length; i++) { | ||
| int64_t start = ArrowArrayViewGetOffsetUnsafe(array_view, array_view->offset + i); | ||
| ok = start == last || (data[start] & 0xC0) != 0x80; | ||
| } |
There was a problem hiding this comment.
It may be worth writing two loops here to avoid the branch in ArrowArrayViewGetOffsetUnsafe (maybe the optimizer does this anyway). Views probably don't benefit from this sort of treatment and can fall back to the generic path.
| // Check that every non-null value of a string or large string array is valid UTF-8. | ||
| // Like Arrow C++, values are checked one at a time (a code point split across two | ||
| // values is invalid) and the bytes of null values are not checked. |
There was a problem hiding this comment.
Should probably update this to reflect what actually happens here (null values are checked, but invalid utf8 in a null slot doesn't fail validation)
| // Returns the length of the longest prefix of data that is valid UTF-8 (RFC 3629), so | ||
| // data is valid UTF-8 if and only if the result is size_bytes. Overlong encodings, | ||
| // surrogates (U+D800 to U+DFFF), code points above U+10FFFF, stray continuation bytes | ||
| // and truncated sequences are all invalid. | ||
| static int64_t ArrowUtf8ValidPrefix(const uint8_t* data, int64_t size_bytes) { |
There was a problem hiding this comment.
Can you add a reference to the Arrow C++ source from which this was derived (or another source if it came from somewhere else?)
This branch has not been deployed
3 participants
NANOARROW_VALIDATION_LEVEL_FULLis documented as the level for untrusted input, but it never checked that string values are valid UTF-8. Arrow C++'sValidateFull()does, so a consumer that relies on FULL to reject bad input accepts strings that Arrow C++ rejects.FULL validation now checks that every non-null
stringandlarge_stringvalue is valid UTF-8 (RFC 3629). Overlong encodings, surrogates (U+D800 to U+DFFF), code points above U+10FFFF, stray continuation bytes and truncated sequences are rejected. Binary types and the levels below FULL are unchanged.string_viewis left for a follow-up PR, because its out-of-line values can't be read safely until FULL also checks view bounds.FULL now rejects data it used to accept. That's the point of the change, but a caller that validated non-UTF-8 bytes in a string array at FULL will now get
EINVAL.The behaviour follows Arrow C++ (checked against pyarrow 25.0.1):
Errors name the value, e.g.
[1] Expected valid UTF-8 string value but found invalid sequence at byte 2.The validator is portable C with no new dependency. It skips ASCII eight bytes at a time. It first validates the whole referenced byte range in one pass and checks that each value starts on a code point boundary (not a continuation byte). That is equivalent to checking each value, but avoids a call per value for short strings. Only if that fails does it check the non-null values one at a time to report the first invalid one.
Cost of FULL validation over 10 MB, compared with
mainand with Arrow C++ValidateFull()(pyarrow 25.0.1) on the same machine (Linux aarch64, Neoverse-N1, Release build):mainTests:
ArrayViewTestValidateUtf8covers string and large_string. It checks valid one- to four-byte sequences, each class of invalid sequence above (with and without a null in the array), split code points, invalid bytes in null values, slices, that DEFAULT still accepts invalid UTF-8, and that binary types are not checked.ctestpasses (Linux aarch64, Debug, 870 tests;NanoarrowIntegrationTestSchemahits the known shared-temp-file collision underctest -jfixed by #969 and passes when run alone), including under ASan/UBSan with clang (395 tests). The changed files are formatted with clang-format 16.0.6, the pre-commit version.This PR's code and description were written by AI (Claude Code) and reviewed by a human maintainer.
🤖 Generated with Claude Code