search

msys2 / msys2-pacman

A friendly fork of https://gitlab.archlinux.org/pacman/pacman
GNU General Public License v2.0
21 stars 12 forks source link

Fix executable file ownership check #37

Open radioflash opened 4 months ago

radioflash commented 4 months ago

If we check ownership for a file we find in the system PATH then we might need a .exe suffix on the full path we identified, because although MSYS will confirm the files existence even if the suffix is missing, we need the "correct" path for the actual ownership check.

See issue https://github.com/msys2/msys2-pacman/issues/36 for details.

Examples

This used to fail:

❯ pacman -Qo ntldd
/ucrt64/bin/ntldd.exe is owned by mingw-w64-ucrt-x86_64-ntldd r19.7fb9365-3

Fully specifying the executable name still works:

❯ pacman -Qo ntldd.exe
/ucrt64/bin/ntldd.exe is owned by mingw-w64-ucrt-x86_64-ntldd r19.7fb9365-3

"Correct" file is checked if there is a wrapper AND an exe file with the same name:

❯ pacman -Qo luajit
/ucrt64/bin/luajit is owned by mingw-w64-ucrt-x86_64-luajit 2.1.1707061634-1
❯ pacman -Qo luajit.exe
/ucrt64/bin/luajit.exe is owned by mingw-w64-ucrt-x86_64-luajit 2.1.1707061634-1

Ownership check fails if an exe extension is erroneously specified (no xzcmp.exe exists):

❯ pacman -Qo xzcmp.exe
error: No package owns xzcmp.exe

Performance impact should be negligible; this has no effect if there is any path separator in the query, and the worstcase is basically an additional lstat call.

radioflash commented 4 months ago

Any feedback on this? This is a fix/improvement that I would personally love to see merged.

It fixes the primary use of pacman -Qo (i.e. "which package, if any, provides < specific command >?") instead of erroring out in a somewhat confusing manner (See linked issue #36 for details).

Is there anything I could do to get this accepted? I'm fully prepared to take care of the MSYS2-packages pull request if this is accepted!

radioflash commented 4 months ago

This does only handle pacman -Qo ntldd and not pacman -Qo /ucrt64/bin/ntldd, right?

Thats correct. Right now the changes should be least-intrusive; I believe I would need either two additional lstats for every queried file, or to retry failed ownership checks (with .exe).

edit: Currently only queries that do PATH lookup anyway are affected

Maybe my performance-concerns are completely misguided here, and I should just make the pacman -Qo /ucrt64/bin/ntldd check also work? Just tell me and I'll update the PR!

Thanks for the review!