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
weechat: prevent impure use of system Python #64262
Conversation
The |
I’ve added a commit to patch all of the PATHS lines. As far as the runtime issue, I imagine that Weechat is just calling whichever Python, etc. are on the PATH. Do you think I should try to patch the C code itself to hardcode the paths instead? |
Thanks a lot for working on this! I'll try to reserve some time tomorrow to actually review this :) |
I've had a quick look at the changes, and they seem good. Not tested it though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the actual change looks good, I'm not 100% sure if we actually need to patch the PATH variables after fixing the paths in the CMake files. May I ask if you built with or without a sandbox enabled?
After skimming through the sources of the weechat plugin structure I think that weechat is actually compiled against the interpreter's C APIs (at least Python, TCL and Ruby invoke their C API), however I'm not that experienced with those, so it's still possible that somehow e.g. the system-wide python is used and I missed something.
Actually we don't need to fix the cmake files, at least not this way. In cmake setup hook we set [1] CMAKE_PREFIX_PATH for each derivation from [1]
[2] https://cmake.org/cmake/help/v3.14/command/find_program.html |
Ah of course, I missed that our cmake wrapper is actuall smart enough to do this 😅
But with |
OK. It sounds like the CMake changes should be backed out. (For what it’s worth, I tried reverting the other commit, keeping only the CMake changes, and the resulting program did crash on launch as described in the original issue. So it makes sense that there’s not actually a CMake-related problem here and that the issue lies elsewhere.) |
b52b891
to
30ff641
Compare
FWIW, I’ve confirmed that the problem still occurs on an Ubuntu system with the latest master version of nixpkgs (663542a), even if I build with sandboxing turned on. I’ve reverted the CMake changes that were in this PR. Is there anything else that needs to be done for this? |
For future reference: #37622 makes a similar change plus another related change. It’s old enough now that it has merge conflicts, though. @Ma27 @the-kenny @lovek323 @lheckemann Is this PR good to be merged at this point? |
@GrahamcOfBorg build weechat |
Motivation for this change
Weechat was calling the system Python at runtime (see #64167). In my case, this resulted in it crashing pretty much immediately on launch. My solution was to have the wrapper script prepend the Nix-provided-Python’s
bin
directory to thePATH
before invoking the wrapped Weechat binary.Questions
Would the same problem exist for the other script interpreters? We’re currently adding the “right” Python and Perl to the
PATH
, but should I adjust this PR to do the same for TCL, Ruby, Guile, and Lua?While working on this patch I noticed that the FindPython.cmake file has a couple of stanzas likeAny idea if this is also a potential source of impurity? Or does Nix’s CMake infrastructure take care of it somehow?I’ve addressed this by adding another commit to the PR.Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)cc @FRidh
Resolves: #37622