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

weechat: prevent impure use of system Python #64262

Merged
merged 1 commit into from Aug 8, 2019

Conversation

bdesham
Copy link
Contributor

@bdesham bdesham commented Jul 3, 2019

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 the PATH before invoking the wrapped Weechat binary.

Questions
  1. 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?

  2. While working on this patch I noticed that the FindPython.cmake file has a couple of stanzas like

    find_program(PYTHON_EXECUTABLE
        NAMES python3.7 python3.6 python3.5 python3.4 python3.3 python3.2 python3.1 python3.0 python3 python2.7 python2.6 python2.5 python
        PATHS /usr/bin /usr/local/bin /usr/pkg/bin
    )
    

    Any 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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

cc @FRidh
Resolves: #37622

@veprbl
Copy link
Member

veprbl commented Jul 3, 2019

@veprbl
Copy link
Member

veprbl commented Jul 8, 2019

The PYTHON_EXECUTABLE is only using to run python from cmake during the build. It won't be a problem for a sandboxed build, but we should be patching out this "PATHS /usr/bin /usr/local/bin /usr/pkg/bin" for systems that don't enable sandboxing. Regarding the issue itself, it would be nice to figure out how the wrong python is called and patch that inside the weechat derivation, not just in nixos module.

@bdesham
Copy link
Contributor Author

bdesham commented Jul 8, 2019

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?

@Ma27
Copy link
Member

Ma27 commented Jul 8, 2019

Thanks a lot for working on this! I'll try to reserve some time tomorrow to actually review this :)

@lheckemann
Copy link
Member

I've had a quick look at the changes, and they seem good. Not tested it though.

Copy link
Member

@Ma27 Ma27 left a 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.

@veprbl
Copy link
Member

veprbl commented Jul 9, 2019

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 buildInputs. If we look at the cmake documentation [2], the CMAKE_PREFIX_PATH is searched before searching $PATH. So, in principle, everything should be fine as long as we have all necessary dependencies in buildInputs. Which we seem to have except for missing libiconv.

[1]

addCMakeParams() {
addToSearchPath CMAKE_PREFIX_PATH $1
}

[2] https://cmake.org/cmake/help/v3.14/command/find_program.html

@Ma27
Copy link
Member

Ma27 commented Jul 9, 2019

Ah of course, I missed that our cmake wrapper is actuall smart enough to do this 😅

So, in principle, everything should be fine as long as we have all necessary dependencies in buildInputs.

But with pythonSupport = true (default for weechat-unwrapped), python is already a build input when compiling weechat, right? (python is a build input for the python plugin of weechat and all plugin's build inputs are used during the build, the plugin simply lands in a different output path).

@bdesham
Copy link
Contributor Author

bdesham commented Jul 10, 2019

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.)

@bdesham bdesham force-pushed the weechat-hide-system-python branch from b52b891 to 30ff641 Compare July 12, 2019 22:21
@bdesham
Copy link
Contributor Author

bdesham commented Jul 19, 2019

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?

@bdesham
Copy link
Contributor Author

bdesham commented Aug 1, 2019

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?

@veprbl
Copy link
Member

veprbl commented Aug 8, 2019

@GrahamcOfBorg build weechat

@veprbl veprbl merged commit aaf6f09 into NixOS:master Aug 8, 2019
@bdesham bdesham deleted the weechat-hide-system-python branch August 8, 2019 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants