Skip to content

fix(windows): set UTF-8 environment values through wide CRT - #2290

Open
knewstimek wants to merge 2 commits into
DeusData:mainfrom
knewstimek:pr/windows-setenv-utf8
Open

knewstimek wants to merge 2 commits into
DeusData:mainfrom
knewstimek:pr/windows-setenv-utf8

Conversation

@knewstimek

@knewstimek knewstimek commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

On Windows, set UTF-8 environment values through _wputenv_s and keep SetEnvironmentVariableW for the process environment. Narrow getenv then returns ANSI-code-page bytes, so the worker marker/quarantine readers and the CLI cache-directory save now use cbm_safe_getenv to keep their paths in UTF-8.

The Windows platform test covers a UTF-8 value rejected by the old setter under a legacy ANSI code page and verifies the wide, UTF-8, and narrow representations. A new extraction regression sets a non-ASCII marker path, calls the actual journal writer, and verifies both start and completion records in that file. A focused before/after journal harness fails with the previous reader and passes with this change.

Local Windows x64 validation with LLVM-MinGW 20260908 / clang 23.1.1: the platform and extraction suites pass (400 passed, 4 platform skips). The CLI suite has 289 passed, 7 platform skips, and four installer failures caused by inherited ACL checks; a control build of the previous PR head reproduces the same four failures at the same assertions. Changed sources compile with -Werror.

Split from #2268.

Signed-off-by: News <knewstimek@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@knewstimek

Copy link
Copy Markdown
Contributor Author

test-diag failed in the unchanged Linux pipeline_python_cross_module_call test (tests/test_pipeline.c:8060, missing CALLS edge). This PR changes only the Windows cbm_setenv branch and its Windows test. I tried rerunning the failed jobs, but GitHub requires repository admin rights. Could you rerun them? I can investigate further if it repeats.

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for splitting #2268 as asked; this piece is small and focused, which made it easy to check where the values go afterwards.

One problem before it merges. With _wputenv_s, a narrow getenv returns the value in the ANSI code page rather than UTF-8, and several in-tree callers read variables set through cbm_setenv with a raw getenv and treat the result as UTF-8:

  • internal/cbm/cbm.c reads CBM_INDEX_MARKER_FILE and CBM_INDEX_QUARANTINE_FILE with getenv and opens them with cbm_fopen; the supervisor sets both in worker_set_local_env.
  • src/cli/cli.c saves getenv("CBM_CACHE_DIR") after main.c has set it, then restores it through cbm_setenv, so ANSI bytes get written back as if they were UTF-8.

With a non-ASCII cache path, crash recovery's marker and quarantine files and the CLI cache-dir restore would break, where today they work whenever _putenv_s accepts the bytes.

What would land:

  1. Switch those readers to cbm_safe_getenv, which returns UTF-8 on Windows.
  2. Add a Windows test that covers a caller: for example, set a non-ASCII marker path through the supervisor's setter and show cbm_fopen opens it, or show the CLI cache-dir save and restore round-trips.

The red test-diag job is pipeline_python_cross_module_call, a known flake on our side, and your diff is entirely inside #ifdef _WIN32. Thank you again.

Signed-off-by: News <knewstimek@users.noreply.github.com>

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @knewstimek. This is exactly what we hoped for. All three readers now go through cbm_safe_getenv (cbm.c marker and quarantine readers, cli.c cache-dir save/restore), and the new extraction test drives the real journal writer with a Δ/丁 path. So it checks the path the supervisor actually uses during crash recovery, not just the setter, and that's what lets us merge this with confidence. Thanks as well for the control build against the previous head for the installer ACL failures. It saved us from chasing them.

We checked every reader of the four variables cbm_setenv now writes. The only raw getenv left is CBM_INDEX_SINGLE_THREAD in pipeline.c, which only ever holds "1", so ASCII and ANSI agree and it's fine. Children are spawned with the inherited wide environment block, so they see the same values.

Approved. Our PR CI doesn't run the Windows unit suites, so before merging we'll run extraction and platform on our Windows VM, including a revert check of the marker reader, to see the new test catch the bug.

Three small notes, none blocking:

  • The 4 KiB buffer in cli.c means an over-long CBM_CACHE_DIR is now treated as absent, so it gets unset on restore rather than restored. That's fine in practice; it's just worth knowing about.
  • The quarantine reader has the same shape as the marker reader but no test of its own. A twin test would be welcome in a follow-up.
  • The narrow-getenv assertion in test_platform.c pins UCRT's own conversion behaviour. That holds on our toolchains, but would change meaning under a UTF-8 activeCodePage manifest.

Thank you again for splitting #2268 so cleanly!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants