py_binary rules default to importing non-sandboxed code

[FuegoFro]

Description of the problem / feature request:

Python (2.7.15 in this case) populates the first entry of the sys.path by resolving the first argument it is given to its real path (including resolving symlinks) and then using the directory containing that real path. This interferes with Bazel's sandboxing of Python files via symlinks and the runfiles in general, since the actual directory from the original source code will be the first entry in the sys.path.

More info on the method of populating sys.path[0] can be found in these discussions:
https://bugs.python.org/issue6386
https://bugs.python.org/issue17639

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Init a fresh git repo, git apply this patch, and run bazel run py:test

diff --git a/WORKSPACE b/WORKSPACE
new file mode 100644
index 0000000..e69de29
diff --git a/py/BUILD b/py/BUILD
new file mode 100644
index 0000000..ec84ab9
--- /dev/null
+++ b/py/BUILD
@@ -0,0 +1,6 @@
+py_binary(
+        name = 'test',
+        srcs = glob(['**/*.py'], exclude=['foo/baz.py']),
+        imports = [''],
+        main = 'main.py',
+)
diff --git a/py/foo/__init__.py b/py/foo/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/py/foo/bar.py b/py/foo/bar.py
new file mode 100644
index 0000000..e69de29
diff --git a/py/foo/baz.py b/py/foo/baz.py
new file mode 100644
index 0000000..e69de29
diff --git a/py/main.py b/py/main.py
new file mode 100644
index 0000000..5c21a2d
--- /dev/null
+++ b/py/main.py
@@ -0,0 +1,10 @@
+import sys
+# sys.path = sys.path[1:]  # Uncommenting this line gives us the expected behavior
+print sys.path
+
+import foo.bar
+print foo.bar.__file__
+
+# This shouldn't work!
+import foo.baz
+print foo.baz.__file__

Note that when sys.path is printed out, the first entry is in the original source code, and not the bazel output directory. Also note that the py/foo/baz.py file isn't included in the target but can be imported. Manually trimming sys.path at the beginning of the main.py file yields the expected behavior.

On solution to this is to not symlink, but rather actually copy, the main file. There are likely other solutions (maybe something involving a .pth file?) but I wasn't able to determine if it was possible to turn off this symlink-following behavior in Python (it seems like not, given the conversations I linked to earlier).

What operating system are you running Bazel on?

macOS 10.14.2

What's the output of bazel info release?

release 0.20.0-homebrew
(though I also repro'd this with non-Brew 0.21.0 and Brew 0.17.2)

Have you found anything relevant by searching the web?

Python issues listed above, regarding sys.path[0]:
https://bugs.python.org/issue6386
https://bugs.python.org/issue17639
Bazel issue regarding symlinking to a runfiles:
#4022
Commit handling runfiles symlinks on Windows:
#6036

[brandjon]

Thanks for the summary. Another consequence of this issue is #793, where code that relies on importing unqualified names from the same directory as the script will fail depending on whether the script is generated.

My guess is that the easiest solution is for us to hack around this in the stub file somehow.

[brandjon]

It looks like sys.argv[0] initialization happens after site.py processing is done. This means that injecting a custom sitecustomize.py into the runfiles (#7959) could not by itself prevent the source dir of the entry point from being added to sys.path.

Looking at the CPython source, apparently sys.argv[0] is not set if the interpreter is launched with a module name (-m) or command (-c) rather than a script. So if we launched the entry point with -c it might work around this, but at that point we may as well just invoke a wrapper module that completely hacks up sys.path to our liking.

[Globegitter]

It seems we are running into a similar issue when trying to auto-generate py_binary targets for console_script entry points of pip packages (see apt-itude/rules_pip#13). It seems to be quite common practice to have the file of the binary target to be the same as the package name. E.g. luigi has a bin/luigi.py, or pytest a bin/pytest.py. When trying to execute these targets you get an error such as module luigi not found or similar. See a minimal repro case: https://github.com/Globegitter/py_repro

Right now we have to patch the packages to rename these files, e.g. to bin/luigi-bin.py, or instead of depending directly on the binary in the external repository, we can create a py_binary target in our own workspace basically recreating what the binary would do.

It would be nice not having to do this workaround and just work out of the box with these pip packages.

[thekyz]

Is there any plan to merge proposed fixes for that issue? This is really bad as it completely breaks sandboxing for python binaries and libs.

[karantza]

Just wanted to add, for anyone who comes across this like me, that this bug also affects py_test. We have code that we've always compiled into .pars, which works fine, but we're now adding tests and noticed this issue when running bazel test. I was able to work around it for now by explicitly popping the first element of sys.path before importing my modules, like the commits above that referenced this issue, but that's really ugly.

[FuegoFro]

Looks like this was fixed by #9250, which I also confirmed locally on Bazel 1.2.1! Thank you for that @alex-xnor and @brandjon! 🎉

[FuegoFro]

Oh no, looks like I spoke too soon! I was bamboozled since I came back to this after not having looked at it for a while. The test I was running was importing code from a different module and hence not seeing these issues.

The wrapper script that was updated does indeed prune sys.path[0], however we then do a subprocess.call/os.execv (depending on the OS) which re-invokes Python which in turn re-does the logic to populate the path and adds our directory to sys.path[0], so that does still remain on the path and by-default break us out of the sandbox 🙁

[thekyz]

How can this be P3? This is a major issue when assessing rebuildability of a py_test or py_binary target. Is there any plan to actually fix this?

[moyamo]

Passing --build_python_zip to bazel seems to work around this issue

[limdor]

What is the status of this issue? Like @thekyz I would also like to challange this bug being P3. One of the basic principles of Bazel is hermeticity and correctness. With this bug sandoboxing in Python is simply broken.

[alexeagle]

Note that we solved this for NodeJS by patching the stdlib to make bazel's sandbox appear as regular files rather than symlinks.

https://github.com/bazelbuild/rules_nodejs/tree/stable/packages/node-patches

[jablin]

Same problem in Ruby.

diff --git a/BUILD b/BUILD
new file mode 100644
index 0000000..8fcdabd
--- /dev/null
+++ b/BUILD
@@ -0,0 +1,6 @@
+genrule(
+  name = "rb",
+  srcs = ["demo"],
+  outs = ["dummy"],
+  cmd = "$(location demo) > $@",
+)
diff --git a/WORKSPACE b/WORKSPACE
new file mode 100644
index 0000000..e69de29
diff --git a/demo b/demo
new file mode 100755
index 0000000..3bb59d3
--- /dev/null
+++ b/demo
@@ -0,0 +1,4 @@
+#!/usr/bin/ruby
+
+require_relative 'lib'
+puts(fn1)
diff --git a/lib.rb b/lib.rb
new file mode 100644
index 0000000..fb69544
--- /dev/null
+++ b/lib.rb
@@ -0,0 +1,3 @@
+def fn1
+  'lib'
+end

This rule should not work because lib.rb is not a declared dependency. Still, the rule works with processwrapper, linux-sandbox and sandboxfs.

I bet it's the same problem with Perl if you use FindBin.

This problem is unsettling.

[jablin]

I think that assigning this problem team-Rules-Python is not a good choice. Isn't this problem about all local execution and sandboxing?

[larsrc-google]

The initial example confused me, as it uses blaze run, which doesn't use sandboxing. But with the modified BUILD file below, it does show this issue in a sandbox when run with bazel build py:runit --spawn_strategy=sandboxed, giving the output ['/usr/local/google/home/larsrc/git/issue-7091/py', '/usr/local/google/home/larsrc/.cache/bazel/_bazel_larsrc/337e8539277b8db2a083a0be14a325a0/sandbox/linux-sandbox/1/execroot/__main__/bazel-out/host/bin/py/test.runfiles', '/usr/local/google/home/larsrc/.cache/bazel/_bazel_larsrc/337e8539277b8db2a083a0be14a325a0/sandbox/linux-sandbox/1/execroot/__main__/bazel-out/host/bin/py/test.runfiles/__main__/py', '/usr/local/google/home/larsrc/.cache/bazel/_bazel_larsrc/337e8539277b8db2a083a0be14a325a0/sandbox/linux-sandbox/1/execroot/__main__/bazel-out/host/bin/py/test.runfiles/__main__', '/usr/local/google/home/larsrc/.cache/bazel/_bazel_larsrc/337e8539277b8db2a083a0be14a325a0/sandbox/linux-sandbox/1/execroot/__main__/bazel-out/host/bin/py/test.runfiles/bazel_tools', '/usr/lib/python39.zip', '/usr/lib/python3.9', '/usr/lib/python3.9/lib-dynload', '/usr/local/lib/python3.9/dist-packages', '/usr/lib/python3/dist-packages', '/usr/lib/python3.9/dist-packages']

$ cat py/BUILD
py_binary(
    name = "test",
    srcs = glob(
        ["**/*.py"],
        exclude = ["foo/baz.py"],
    ),
    imports = [""],
    main = "main.py",
)

genrule(
    name = "runit",
    outs = ["dummy"],
    cmd = "$(location :test) > $@",
    tools = [":test"],
)

[tpudlik]

This bug interacts particularly perniciously with py_proto_library, which uses Python imports not qualified with the repo name (which should not be allowed, #7067). The result is that imports in proto generated code may become ambiguous and resolve to incorrect packages at runtime (see https://issuetracker.google.com/issues/241456982#comment8 for an example and more details).

[tpudlik]

This bug was also reported as bazelbuild/rules_python#382, and f99cebe fixed it for Python 3.11+ (but note that Python 3.11 is not released yet, it's only coming in October).

[rickeylev]

I had an idea for another possible solution here. It's not pretty, but should contain edits to the stub template and work with earlier versions.

Basically, use a combination of compile(), exec() and sys.modules (maybe __name__ and __loader__, too) to have some shim code that modifies sys.path, and then loads the originally intended main file, futzing with e.g. sys.modules to make sure the original main sees itself as __main__.

I'm pretty sure I got the idea here from PAR files loading things from zip files. It's implemented in Python, but has to play tricks to load the original main and trigger the if name == main block.

[larsrc-google]

I take it @jvolkman 's PR fixed this.

[jvolkman]

@larsrc-google that PR was never merged (it wasn't mine; I just commented). I think #15701 fixes this, but only for Python 3.11+.

[matts1]

It appears I'm missing permissions to reopen the issue.

Could someone else do that?