fix(reading): never decode strings as ASCII - #170
Merged
Apollo3zehn merged 1 commit intoAug 14, 2026
Conversation
ReadFixedLengthString and ReadNullTerminatedString took `CharacterSetEncoding encoding = CharacterSetEncoding.ASCII`, and most callers inherited that default. Encoding.ASCII replaces every byte >= 0x80 with '?', so any non-ASCII payload was silently and irrecoverably corrupted on read — and the result is indistinguishable from data that legitimately contains '?'. Two observable failures, both reproduced against the released 2.1.4: - A fixed-length string returns one '?' per non-ASCII byte, for datasets, compound members and attribute values, even though the bytes on disk are valid UTF-8 and the datatype message declares H5T_CSET_UTF8. Because the substitution is per byte the string's length changes too, so callers doing their own width arithmetic also get a wrong answer. - A soft link whose target path contains non-ASCII characters no longer resolves: the mangled path does not name the object it points at, and Get() returns an unresolved-link stub. The link name itself decodes correctly, so the target group is reachable directly while the link to it is broken. Remove the parameter and always decode UTF-8. H5T_cset_t defines only ASCII and UTF-8; ASCII is a strict subset, so this is byte-for-byte identical for every conformant payload, including the fields the specification fixes as ASCII. Where the two differ is a payload holding UTF-8 while declared — or defaulted — to ASCII, which is what any writer that does not set a character set produces: UTF-8 recovers it, and marks genuinely malformed bytes U+FFFD rather than an undetectable '?'. This also matches GetDecodeInfoForVariableLengthString, which already hardcoded UTF-8 and is the reason variable-length strings were unaffected. Beyond the two failures above, the same default silently applied to compound and enum member names, object comments, opaque tags, virtual-dataset source file and dataset paths, external link paths, and multi-file driver member names. ReadNullTerminatedString measured its padding from the decoded string's length rather than the bytes read. Those are equal only under ASCII, so with UTF-8 a multi-byte character would have desynchronised the driver; it now counts bytes. The character set fields are still read to keep the driver aligned, and CharacterSetEncoding is still used on the write side. Closes Apollo3zehn#169 Co-Authored-By: Claude <noreply@anthropic.com>
Blackclaws
force-pushed
the
fix/fixed-length-string-utf8
branch
from
July 29, 2026 23:27
3b4147e to
6f94a52
Compare
This was referenced Jul 29, 2026
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 #169.
Problem
ReadUtils.ReadFixedLengthStringandReadUtils.ReadNullTerminatedStringdeclaredCharacterSetEncoding encoding = CharacterSetEncoding.ASCII, and most call sites inherited that default.Encoding.ASCIIreplaces every byte>= 0x80with?, so any non-ASCII payload was corrupted on read — silently, irrecoverably, and indistinguishably from data that legitimately contains?.Two observable failures, both reproduced against released 2.1.4 and against this PR's base commit:
1. Fixed-length strings.
"5µm→x😀"(7 chars / 12 UTF-8 bytes) round-tripped through PureHDF:5??m???x????5µm→x😀5??m???x????5µm→x😀5??m???x????5µm→x😀5µm→x😀5µm→x😀5µm→x😀5µm→x😀The bytes on disk are valid UTF-8 and the datatype message declares
H5T_CSET_UTF8— h5py reads the same files back correctly. Because the substitution is per byte, the string's length changes too (7 in, 12 out), so a caller doing its own width arithmetic also gets a wrong answer.2. Soft links with a non-ASCII target no longer resolve. The link name decodes correctly, so the target is reachable directly while the link to it is broken:
Beyond those two, the same default silently applied to compound and enum member names, object comments, opaque tags, virtual-dataset source file and dataset paths, external link paths, and multi-file driver member names.
Change
Remove the parameter; always decode UTF-8.
H5T_cset_tdefines onlyH5T_CSET_ASCIIandH5T_CSET_UTF8, and ASCII is a strict subset of UTF-8 — all 128 ASCII byte values decode identically under both and re-encode byte-identically — so this cannot change the result for a conformant payload, including the fields the format specification fixes as ASCII (filter names, driver identifiers, the old modification-time digits).Where the two differ is a payload holding UTF-8 while declared, or defaulted, to ASCII — which is what any writer that does not set a character set produces. I checked that case with h5py-written files that declare
H5T_CSET_ASCIIwhile holding UTF-8 bytes: dispatching on the declared character set still returns5??m???x, while decoding UTF-8 returns5µm→x. That is why this removes the option rather than threadingbitField.Encodingthrough — honouring the declaration would leave those files broken, andEncoding.ASCIIhas no case in which it is preferable. When bytes genuinely are malformed, UTF-8 yields U+FFFD, which is at least detectable.It also matches
GetDecodeInfoForVariableLengthString, which already hardcodedEncoding.UTF8.GetString— the reason variable-length strings were unaffected throughout.Two details worth flagging for review:
ReadNullTerminatedStringmeasured its padding from the decoded string's length, not the bytes read. Those are equal only under ASCII, so switching to UTF-8 would have desynchronised the driver on any multi-byte character. It now counts bytes. This was a latent bug for any caller that passed UTF-8 explicitly withpad: true._ = driver.ReadByte()) to keep the driver aligned, andCharacterSetEncodingis still used on the write side, so the enum stays.Tests
Two new files in
tests/PureHDF.Tests/RoundTrip/:DatasetTests@FixedLengthString.cs— fixed-length dataset, compound member and attribute round trips, plus a guard on the variable-length path so a future change cannot regress it.LinkTests@NonAsciiTarget.cs— soft link resolution across an ASCII control and 2-, 3- and 4-byte UTF-8 targets.Against the base commit: 6 of 8 fail; the 2 that pass are the variable-length guard and the ASCII control. With this change: 8/8, and the full suite is 518 passed, 0 failed, 10 skipped (HSDS, no network) on a clean rebuild, no new warnings.
Not addressed here
Encoding.ASCII.GetBytesis still used on the write side for compound member names, enum member names, opaque tags and filter names — so a non-ASCII name is destroyed on write, before any reader is involved. That is a separate change with a different risk profile (it alters what PureHDF produces), andCompoundPropertyDescription.Encodealready carries your// TODO is this really ASCII? The spec does not specify it but does it so for enumerated data type (there it is ASCII). Happy to open a follow-up issue or PR if you want it — it seemed like your call to make rather than mine.