[2.24.0] bash: Symlinks generated in /dev/ is broken in Japanese env

[shinfd]

• I was not able to find an open or closed issue matching what I'm seeing

Setup

• Which version of Git for Windows are you using? Is it 32-bit or 64-bit?

git version 2.24.0.windows.1
cpu: x86_64
built from commit: 6a3fa2a1833f3c7ac71cefefe44b3a4b4138e220
sizeof-long: 4
sizeof-size_t: 8

• Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?

Microsoft Windows [Version 10.0.17134.1099]

• What options did you set as part of the installation? Or did you choose the
  defaults?

Editor Option: Nano
Custom Editor Path:
Path Option: Cmd
SSH Option: OpenSSH
Tortoise Option: false
CURL Option: OpenSSL
CRLF Option: CRLFAlways
Bash Terminal Option: ConHost
Performance Tweaks FSCache: Enabled
Use Credential Manager: Disabled
Enable Symlinks: Disabled
Enable Builtin Interactive Add: Disabled

• Any other interesting things about your environment that might be related
  to the issue you're seeing?

Running in Japanese windows environment (which uses CP932 as its codepage)

Details

• Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

git-bash.exe

• What commands did you run to trigger this issue? If you can provide a
  Minimal, Complete, and Verifiable example
  this will help us understand the issue.

$ ls -l /dev/fd /dev/stdin /dev/stdout /dev/stderr

• What did you expect to occur after running these commands?

lrwxrwxrwx 1 1049089 3 Nov  5 16:07 /dev/fd -> /proc/self/fd
lrwxrwxrwx 1 1049089 3 Nov  5 16:08 /dev/stderr -> /proc/self/fd/2
lrwxrwxrwx 1 1049089 3 Nov  5 16:08 /dev/stdin -> /proc/self/fd/0
lrwxrwxrwx 1 1049089 3 Nov  5 16:08 /dev/stdout -> /proc/self/fd/1

• What actually happened instead?

lrwxrwxrwx 1 1049089 3 Nov  5 15:26 /dev/fd -> yt/
lrwxrwxrwx 1 1049089 3 Nov  5 15:26 /dev/stderr -> yt/
lrwxrwxrwx 1 1049089 3 Nov  5 15:26 /dev/stdin -> yt/
lrwxrwxrwx 1 1049089 3 Nov  5 15:26 /dev/stdout -> yt/

Further Details

All symlinks generated in /dev/ via the fix to #2291 are broken. This causes scripts using similar constructs as outlined in the said issue to break, and we are forced to revert back to 2.22.0.

Opening the symlink files with binary editor showed that in place of the 0xFF 0xFE BOM was 0x79 0x74 hence "yt".

Emulating the CreateCygwinSymlink() implementation in iss file, I tried the following code in Inno Setup:

MsgBox('Hello ' + #255 + #254 + ' world' + #13 + #10 + 'Hello ' + Chr(255) + Chr(254) + ' world', mbInformation, MB_OK);

and this is what I get in my environment. Using # results in the same yt.

I am pretty sure I read that # and Chr should be the same thing, but apparently this is not the case here. Don't know if this applies to Pascal in general or is particular to Inno Setup.

Assuming this is functioning okay in many western locales, my guess is that the code page for the script is determined at runtime, and because 0xFF is undefined in CP932 / Shift-JIS, it is translated into something random... or maybe intentional: 0xFF 0xFE in Latin-1 is ӱþ, and someone decided to convert it to similar yet representable letter combo "yt".

Chr() provided desired result in the above sample, but I am not positive if this is a safe solution to ensure that correct bytes are written.

[dscho]

Hmm, that is bad. I thought that constructing an ASCII string in InnoSetup's Pascal would work: git-for-windows/build-extra@02cb1e0#diff-9db0b81cc5d2dc9d49e4e8634ac624b6R2350

Maybe there is a way to construct an AnsiString with 0xff 0xfe in Pascal? I do not have a non-US locale to test with, maybe you can find something?

Alternatively, we will have to compile a helper from C and include that in our installer, then call it instead of trying to write the Cygwin-style symlink directly in Pascal.

Side note: I would have loved to catch an issue like this in our 2-week pre-release period, as I am certain that even Git for Windows v2.24.0-rc0 contained this bug. Maybe I can ask you to test during the next pre-release cycles, to make sure that nothing is broken in Japanese environments?

[dscho]

@shinfd I just had a quick look at StackOverflow, and this seems a good start: https://stackoverflow.com/questions/38617829/writing-binary-file-in-inno-setup. Granted, it tries to convert a long hex string to a string (note the lower-case string, as opposed to the regular String or even AnsiString), and then write it out. But I think we should be able to make use of a similar technique to construct a string.

[dscho]

@crypto-rsa feel free to join in with our happy little ticket trying to fix this (I assume you also have a non-US locale?).

[crypto-rsa]

@dscho I do and I'll be happy to help.

[dscho]

@crypto-rsa perfect. Give me five more minute and I will push up a branch for you to test. Do you have a working Git for Windows SDK?

[crypto-rsa]

@dscho I can't remember installing it deliberately so I suppose the answer is no.

[dscho]

Okay. Can you clone https://github.com/dscho/build-extra and see whether you can run InnoSetup\ISCC.exe in there?

[dscho]

I guess not. I will be checking back in a couple of hours.

[dscho]

@crypto-rsa @shinfd if any of you could please clone https://github.com/git-for-windows/build-extra and then run installer\InnoSetup\ISCC.exe symlink-test.iss after pasting this into a file symlink-test.iss, then call the resulting installer\Output\mysetup.exe and verify that it creates a file that ls -la in Git Bash will report as symlink-test -> symlink-target.txt, that would be awesome:

[Setup]
AppName=SymlinkTest
AppVersion=0.0
WizardStyle=modern
DefaultDirName=C:\My Program
PrivilegesRequired=lowest

[Code]
procedure LogError(Msg:string);
begin
    MsgBox(Msg,mbError,MB_OK);
    Log(Msg);
end;

function GetFileAttributes(Path:PAnsiChar):DWORD;
external '[email protected] stdcall';

function SetFileAttributes(Path:PAnsiChar;dwFileAttributes:DWORD):BOOL;
external '[email protected] stdcall';

function CryptStringToBinary(sz:string;cch:LongWord;flags:LongWord;binary:string;var size:LongWord;skip:LongWord;flagsused:LongWord):Integer;
external '[email protected] stdcall';

const
  CRYPT_STRING_HEX = $04;
  HEX_CHARS = '0123456789abcdef';

function CharToHex(C:Integer):string;
begin
    Result:=HEX_CHARS[((C div 16) and 15)+1]+HEX_CHARS[(C and 15)+1];
end;

function CreateCygwinSymlink(SymlinkPath,TargetPath:String):Boolean;
var
    Attribute:DWord;
    i:Integer;
    Hex,Buffer:string;
    Stream:TStream;
    Size:LongWord;
begin
    Result:=True;

    // assuming that the target is actually all-ASCII, convert to UTF-16
    for i:=Length(TargetPath) downto 1 do
        TargetPath:=Copy(TargetPath,1,i)+#0+Copy(TargetPath,i+1,Length(TargetPath)-i);

    Hex:='213c73796d6c696e6b3efffe'; // "!<symlink>\xff\xfe"
    for i:=1 to Length(TargetPath) do
        Hex:=Hex+CharToHex(Ord(TargetPath[i])); // append wide characters as hex
    Hex:=Hex+'0000'; // append a wide NUL

    // write the file
    Stream:=TFileStream.Create(SymlinkPath,fmCreate);
    try
        Size:=Length(Hex) div 2;
        SetLength(Buffer,Size);
        if (CryptStringToBinary(Hex,Length(Hex),CRYPT_STRING_HEX,Buffer,Size,0,0)=0) or (Size<>Length(Hex) div 2) then
            RaiseException('could not decode hex '+Hex);
        Stream.WriteBuffer(Buffer,Size);
    except
        LogError('Could not write "'+SymlinkPath+'" '+GetExceptionMessage());
        Result:=False;
    finally
        Stream.Free
    end;

    // Set system bit (required for Cygwin to interpret this as a symlink)
    Attribute:=GetFileAttributes(SymlinkPath);
    if (Attribute and 4) = 0  then
    begin
        Attribute:=Attribute or 4;
        if not SetFileAttributes(SymlinkPath,Attribute) then begin
            LogError('Could not mark "'+SymlinkPath+'" as system file');
            Result:=False;
        end;
    end;
end;

function InitializeSetup(): Boolean;
begin
    if CreateCygwinSymlink('symlink-test','symlink-target.txt') then
        MsgBox('Created symlink-test.',mbInformation,MB_OK)
    else
        MsgBox('Failed to create symlink-test.',mbInformation,MB_OK);
    Result:=False;
end;

[shinfd]

Sorry. Got back to work in morning now and found myself a tad too late to help.
I did confirm that the said test has successfully worked to create the symlink file.

Thanks for such a quick response and fix!

Side note: I would have loved to catch an issue like this in our 2-week pre-release period, as I am certain that even Git for Windows v2.24.0-rc0 contained this bug. Maybe I can ask you to test during the next pre-release cycles, to make sure that nothing is broken in Japanese environments?

I will request the person who originally reported this (and the one introducing the new version to our work environment) to try out RCs more actively.

[crypto-rsa]

I can also confirm the installer works as described on Windows with Czech locale.

[shinfd]

Final confirmation that the newly released 2.24.0(2) fixed this problem in our environment.
Thank you again!

[dscho]

Final confirmation that the newly released 2.24.0(2) fixed this problem in our environment.

Excellent, thank you for testing, and reporting!

[SamB]

Hmm. I knew there was a reason I liked the "just create the dev directory, let post-install do the rest" plan...

[dscho]

@SamB so maybe, if you know so much, you also know why the post-install step hangs on so many setups (just look through the existing issues in this here bug tracker, it is not hard to find). And if you do, maybe you care to enlighten the rest of us.

[SamB]

@dscho Okay, so, yes: my previous comment was unmitigated snark, unkind, and uncorrect. I'm sorry.

I suppose in the future I should just ask if there's a reason for what looks to me like an overly-complicated approach, rather than just letting that uneasy feeling fester the way I did this time.

(Also, If I really knew that much, wouldn't I have noticed that SaveStringToFile was locale-dependent? Not that the docs are all that clear on this or anything ...)

[dscho]

@SamB apology accepted.

And I agree that the documentation of InnoSetup is not the easiest to use, and not the most complete.

I'm just happy that we are finally on our way to mitigate those hangs in the post-install script simply by dropping that script. We're not there yet, but we do have the functionality implemented in our InnoSetup definition. I hope that by the time Git for Windows v2.25.0 comes around (which should be about 12 weeks after v2.24.0, i.e. around January 27th, 2020, even if https://tinyurl.com/gitCal does not have it listed yet) we're ready to just drop the /etc/post-install/ part from the installer, and get rid of executing the Bash one time as an administrator.