An old value of MSYS with unpaired surrogates is silently replaced for fixtures
#1574
Labels
Comments
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this issue
Sep 6, 2024
The value of an environment variable as obtained by the facilities in `std::env` is not always well-formed Unicode. Specifically, on Windows the values of environment variables, like paths, are natively UTF-16LE strings except that unpaired surrogate code points can also occur. An `&OsStr` on Windows may accordingly not quite be UTF-8. When the `MSYS` variable is absent, we treat this the same as when it is present but empty. However, as described in GitoxideLabs#1574, an `MSYS` variable that is present but whose value contains an unpaired surrogate would also be replaced entirely, rather than appending to its old value. This changes that, to instead append, retaining whatever was there even if it was ill-formed Unicode. An alternative change could be to panic when the old value is ill-formed Unicode. This commit allows and appends to the old value, rather than panicking or keeping and documenting the previous behavior of discarding the old value, because the appended sequence ` winsymlinks:nativestrict` is effective at causing fixture scripts to attempt to create actual symlinks even if the preceding code point is an unpaired Unicode high surrogate.
Byron
added
help wanted
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Current behavior 😯
In
gix-testtools, when test fixture scripts are run, theconfigure_commandfunction customizes their environment. Since 0899c2e (#1444), this includes setting theMSYSvariable to make it so that commands in shell scripts such asln -swill really attempt to create symlinks rather than copies.If
MSYSwas already set, the options held in its old value are kept, with the new option appended at the end so it will only suppress an option that conflicts with it, otherwise leaving the effects of preceding options in place.But this does not happen if the old value of
MSYScontained an unpaired surrogate code point. This is becauseenv::varrather thanenv::var_osis used, so theunwrap_or_defaultmethod returns an empty string both whenMSYSwas absent and when it was present with a value that could not be represented as well-formed UTF-8.https://github.com/Byron/gitoxide/blob/9bddeeb948397df3d1e391ec48597cc34d10acd0/tests/tools/src/lib.rs#L596-L597
https://github.com/Byron/gitoxide/blob/9bddeeb948397df3d1e391ec48597cc34d10acd0/tests/tools/src/lib.rs#L605
There are at least three scenarios where
MSYSmay contain an unpaired surrogate:error_start:contains one. This seems unlikely, except as a special case of...MSYScontains an unpaired surrogate.The impact of this bug is therefore small. However, fixing it would help clarify the intended semantics of how
gix-testtoolstreats theMSYSenvironment variable when running test fixture scripts. (This may potentially be relevant to some changes made for #1578.)Expected behavior 🤔
Due to the nature of
gix-testtoolsas a testing facility, both as part ofgitoxide's test suite and when the published crate is used, I think any of these behaviors are fine and should be considered to fix the bug:Always panic when the previous value of
MSYScontained an unpaired surrogate, whether or not this is documented.Always accept and append to the previous value of
MSYSwhen it contains an unpaired surrogate, such as by usingenv::var_osand callingpush(rather thanenv::varand callingpush_str), whether or not this is documented.If this is done, then
unwrap_or_defaultcould still be used, because it would only be falling back to the default empty value when the environment variable was absent.Most other behaviors, including the current behavior, if it is documented.
While any of those would, in my opinion, satisfy users' expectations and eliminate the bug as such, I think the best behavior would be to do (1) or (2). Specifically:
nativesymlinksto a preexisting value ofMSYSthat ends in a high surrogate is effective at causing symlinks to be created, then (2) is preferred.I plan to investigate this and open a PR that makes one of those changes. (This investigation has been slightly slowed by the problem described in rust-lang/libz-sys#215, but I should be able to complete it.)
Update: I have opened #1580, which changes it to use
OsStringinstead ofStringand to append withOsString::push. As noted there, I verified that a value ofMSYSproduced this way does still have the desired effect on symlink creation.Git behavior
Not directly applicable.
Steps to reproduce 🕹
The nice way to reproduce this would be to make a fixture that displays the value, or to add a tracing statement, or to use a debugger.
Instead, I just added a
panic!to print out whateverMSYSvalue will be used:Then I ran a test that, while conceptually unrelated to this issue, exercises that code, ensuring the
panic!is hit. The following output was warnings induced by making that change, about unused variables and unreachable code, removed. The full output can be seen in this gist.First, a control, with well-formed Unicode:
A now, when the existing value contains an unpaired high surrogate:
Therefore, normally it is able to keep content from the old value:
But when it cannot be converted to UTF-8, its silently discards it:
(The reason I added the
zafter the unpaired surrogate was to show a situation where it is not at the very end, which is the more common situation. The problem is not limited to when it is at the very end; rather, whether appending ought to be done, rather than panicking, should, I think, be determined by whether program built against the MSYS DLL are able to heed the appendedwinsymlinksoption even when it the space that delimits it follows an unpaired high surrogate.)The text was updated successfully, but these errors were encountered: