Conversation
Under a C/POSIX locale, fgetwc() treats any byte >= 0x80 as an invalid multibyte sequence and returns WEOF, indistinguishable from real EOF. yamldump() and printYAMLStatInfo()'s character-counting loop looped on fgetwc(), so a single non-ASCII byte in captured stdout/stderr silently truncated the rest of the YAML report, while data_truncated was computed purely from byte size and never reflected it. Add yamlgetutf8()/yamlpututf8() to decode/encode UTF-8 by hand from raw bytes, independent of locale. Undecodable bytes (including UTF-8-encoded surrogates, which are never valid) are now skipped instead of stopping the dump, and data_truncated is derived from a first pass that also tracks whether any bytes had to be skipped this way. yamldump() also now re-indents on YAML 1.1's other line-break characters (NEL, LS, PS), which previously could pass through raw and de-indent content out of the literal block once locale no longer masked the issue. Refs #2250
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.
Summary
Under a C/POSIX locale,
fgetwc()treats any byte >= 0x80 as an invalid multibytesequence and returns
WEOF, indistinguishable from real EOF.yamldump()andprintYAMLStatInfo()'s character-counting loop both looped onfgetwc(), so asingle non-ASCII byte anywhere in captured stdout/stderr silently truncated the
rest of the YAML report — while
data_truncatedwas computed purely from bytesize and never reflected it.
yamlgetutf8()/yamlpututf8()inutils.cto decode/encode UTF-8 byhand from raw bytes, independent of locale.
are now skipped instead of stopping the dump, and
data_truncatedis derivedfrom a first pass that also tracks whether any bytes had to be skipped this way.
yamldump()also now re-indents on YAML 1.1's other line-break characters(NEL, LS, PS), which previously could pass through raw and de-indent content
out of the literal block once locale no longer masked the issue (found during
adversarial review — a job could otherwise inject sibling YAML keys into its
own invocation record via a crafted stdout/stderr byte sequence).
Verified with manual repro (valid UTF-8 em-dash case from the issue's repro,
invalid-byte case, CESU-8/WTF-8 surrogate case, and a YAML block-scalar
injection attempt parsed back with PyYAML) plus the full
pegasus-kickstartC test suite.
Skipped review findings
Autonomous review (simplify, security, general-correctness, Codex review,
Codex adversarial-review) surfaced a few additional items that are pre-existing
behavior unrelated to this locale bug, judged out of scope for this fix:
statinfo.c:513-533— the two-pass fd counting/skip loop is duplicated andnow slightly costlier per call than the old
fgetwcversion; predates thischange (same double-pass shape existed before), a single-pass rewrite is a
separate cleanup.
utils.c:88yamlgetutf8— its 3-state return (0/1/2) could be collapsed toa bool, but that would lose the "skip invalid, keep reading" signal this fix
relies on; kept as-is.
statinfo.c:518,536— twoFILE*opened on the same duped fd viafdopen,the first never
fclose()'d; pre-existing pattern (existed withfgetwctoo), not introduced by this diff.
statinfo.c:493-524—-B/data_section_sizeis documented as a byte limitbut the counting loop counts decoded UTF-8 code points, so multi-byte output
can exceed the configured limit by up to ~4x. This predates pegasus-kickstart silently truncates YAML report on non-ASCII bytes under C locale #2250 (
fgetwccounted wide chars identically) — making the limit byte-accurate is a larger
change than this locale fix warrants; worth a follow-up issue.
e2e note
e2e (pipeline 7116) had a single failure unrelated to this change:
Performance Tests: [014-planner-performance-100k, launch-bamboo-test-planner-only]— aJava planner timing test (100k-job workflow planning took 1760s against an
upper limit of 1600s), which doesn't touch pegasus-kickstart at all. Confirmed
via trace as CI-runner timing flakiness, not a regression from this diff.
Test plan
make build-c(pegasus-kickstart, pegasus-cluster, pegasus-keg)packages/pegasus-kickstart/test/test.sh— 37/37 applicable tests pass(the one skipped test requires the Go
pegasus-integritybinary, notbuilt by
make build-cin this environment; unrelated to this change)a CESU-8 surrogate case, and a YAML injection attempt — all verified
fixed
timing flake noted above
Closes #2250