Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix executable file ownership check #37

Open
wants to merge 1 commit into
base: msys2-v6.0.2
Choose a base branch
from

Conversation

radioflash
Copy link

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 #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
Copy link
Author

radioflash commented Feb 28, 2024

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!

Copy link
Member

@lazka lazka left a comment

Choose a reason for hiding this comment

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

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

strcat(fullname, ".exe");
struct stat sbExe;
if (lstat(fullname, &sbExe) == 0) {
if (sbExe.st_ino == bufptr->st_ino) {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick, st_dev needs to also be the same

Copy link
Author

Choose a reason for hiding this comment

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

Fixed!

@@ -70,6 +73,32 @@ static int search_path(char **filename, struct stat *bufptr)

if(lstat(fullname, bufptr) == 0) {
free(*filename);

Copy link
Member

Choose a reason for hiding this comment

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

Please guard all changes via #ifdef __MSYS__ so on Linux the original behavior is used.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

When checking ownership for a command in the system PATH, potentially
append a .exe file extension. MSYS itself tolerates omission, but
pacman would be unable to identify the command owner without the
"correct" path.

This is not done if distinct files exist (with/without .exe).
@radioflash
Copy link
Author

radioflash commented Feb 29, 2024

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!

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.

None yet

2 participants