msys2 / msys2-runtime

Our friendly fork of Cygwin 💖 https://cygwin.org 💖 see the wiki for details
https://github.com/msys2/msys2-runtime/wiki
GNU General Public License v2.0
185 stars 39 forks source link

Skip path conversion for strace arguments #226

Open atrigent opened 2 months ago

atrigent commented 2 months ago

There is already a special case for disabling path conversion with strace in the patch series (https://github.com/msys2/msys2-runtime/commit/916afdca1a3693de1482571e99bef0b1b12998be), but it only applies to environment variables. The message on that commit seems to be acknowledging that conversion of arguments is also problematic, so I'm not sure why it doesn't disable that as well.

dscho commented 2 months ago

The message on that commit seems to be acknowledging that conversion of arguments is also problematic, so I'm not sure why it doesn't disable that as well.

The commit message says that:

Strace is a Windows program [...]

This means that it does not understand any Unix-like paths passed to it as arguments. For example, this invocation would not do what you think it should:

$ strace /usr/bin/uname -a

Because strace is a Windows program, it would interpret /usr/bin/uname as a regular Windows path, i.e. equivalent to \usr\bin\uname, which is an absolute path on the current drive. In most instances, therefore, it would resolve to C:\usr\bin\uname, which typically does not exist.

The only reason why it does work is that the MSYS2 runtime converts the arguments automatically, so that strace never sees the Unix-style path.

Now, why is the environment such a different beast?

The reason is that strace itself does not use the environment variables. But its purpose is to launch MSYS programs which do, and which also understand Unix-like paths such as /usr/bin/uname. Therefore, above-mentioned commit strikes me as trying to avoid unnecessary (and potentially error-prone) back-and-forth conversion of the environment. The same isn't an issue for the arguments, they won't be converted back from Windows paths to Unix paths.

atrigent commented 2 months ago

The only reason why it does work is that the MSYS2 runtime converts the arguments automatically, so that strace never sees the Unix-style path.

Don't you think you should double-check this before saying it? And how exactly are you imagining it works in Cygwin?

https://github.com/mirror/newlib-cygwin/blob/fe2545e9faaf4bf9586f61a7b83d5cb5af501194/winsup/utils/mingw/strace.cc#L344-L345

converted back from Windows paths to Unix paths

Please read https://www.msys2.org/docs/filesystem-paths/#windows-unix-path-conversion

dscho commented 2 months ago

Don't you think you should double-check this before saying it? And how exactly are you imagining it works in Cygwin?

https://github.com/mirror/newlib-cygwin/blob/fe2545e9faaf4bf9586f61a7b83d5cb5af501194/winsup/utils/mingw/strace.cc#L344-L345

I stand corrected regarding the first parameter (*argv, which is strace.exe itself).

I do have admit that I wrote this all from memory and did not try this out (you could, if you wanted, using MSYS2_ARG_CONV_EXCL).

converted back from Windows paths to Unix paths

Please read https://www.msys2.org/docs/filesystem-paths/#windows-unix-path-conversion

It is admittedly not documented very well, but there are environment variables that are converted from Windows to Unix paths when an MSYS program is called from a Windows program.

dscho commented 2 months ago

And how exactly are you imagining it works in Cygwin?

Sorry, I forgot to answer that question: Cygwin plays it safe and does the double-conversion, I expect. I.e. when running strace from a Cygwin Bash (or any other Cygwin process), it converts the subset of the environment that Cygwin cares about (and unlike MSYS2 not all of the environment variables, guessing whether their values are Unix paths or Unix path lists) from Unix to Windows paths, then strace spawns the Cygwin process which means that the reverse conversion happens.

atrigent commented 2 months ago

I stand corrected regarding the first parameter (*argv, which is strace.exe itself).

My dude, I beg of you, please read the code. Literally just read the function name. This is the code for launching the child process. It is not modifying its own argv[0].

It does a similar conversion for handling the output file path:

https://github.com/mirror/newlib-cygwin/blob/fe2545e9faaf4bf9586f61a7b83d5cb5af501194/winsup/utils/mingw/strace.cc#L1123

dscho commented 2 months ago

I stand corrected regarding the first parameter (*argv, which is strace.exe itself).

[...] This is the code for launching the child process. It is not modifying its own argv[0].

Oh right! And it also makes sense because we're looking at the Cygwin code directly, without the MSYS2-specific modifications. And Cygwin does not convert the command-line arguments, therefore its strace.exe obviously has to do that itself.

Back to your question, though, whether MSYS2's strace should be exempt from the argument conversion: It does not really matter, does it, whether the MSYS2 runtime does it or strace?

I guess, theoretically, that something like strace ssh.exe some-host /usr/bin/sh would benefit from not converting the /usr/bin/sh argument. But it is most likely as easy to construe examples where the conversion would be desirable (see e.g. this comment about strace mingw32-make.exe for inspiration)).