[llvm] Add support for running tests as root #75285
Conversation
There are a few test that check access permissions, so they need to be disabled when running the tests as root. The most common use case for running tests as root is inside of a container. GitHub Actions, for example, only supports running the root user inside of containers, so this change is necessary in order to run the tests inside of a container running in the GitHub Actions environment.
|
@llvm/pr-subscribers-testing-tools @llvm/pr-subscribers-debuginfo Author: Tom Stellard (tstellar) ChangesThere are a few test that check access permissions, so they need to be disabled when running the tests as root. The most common use case for running tests as root is inside of a container. GitHub Actions, for example, only supports running the root user inside of containers, so this change is necessary in order to run the tests inside of a container running in the GitHub Actions environment. Full diff: https://github.com/llvm/llvm-project/pull/75285.diff 5 Files Affected:
diff --git a/llvm/test/tools/llvm-ar/error-opening-permission.test b/llvm/test/tools/llvm-ar/error-opening-permission.test
index 4107bdfc044fe..b42f95329a3c7 100644
--- a/llvm/test/tools/llvm-ar/error-opening-permission.test
+++ b/llvm/test/tools/llvm-ar/error-opening-permission.test
@@ -1,6 +1,7 @@
## Unsupported on windows as marking files "unreadable"
## is non-trivial on windows.
# UNSUPPORTED: system-windows
+# REQUIRES: non-root-user
# RUN: rm -rf %t && mkdir -p %t
# RUN: echo file1 > %t/1.txt
diff --git a/llvm/test/tools/llvm-dwarfdump/X86/output.s b/llvm/test/tools/llvm-dwarfdump/X86/output.s
index 37132eb55ca55..e7c9234ed74cf 100644
--- a/llvm/test/tools/llvm-dwarfdump/X86/output.s
+++ b/llvm/test/tools/llvm-dwarfdump/X86/output.s
@@ -1,3 +1,4 @@
+# REQUIRES: non-root-user
# RUN: rm -f %t1.txt %t2.txt %t3.txt
# RUN: llvm-mc %S/brief.s -filetype obj -triple x86_64-apple-darwin -o %t.o
diff --git a/llvm/test/tools/llvm-ifs/fail-file-write.test b/llvm/test/tools/llvm-ifs/fail-file-write.test
index d5232070c1d03..f13500f226205 100644
--- a/llvm/test/tools/llvm-ifs/fail-file-write.test
+++ b/llvm/test/tools/llvm-ifs/fail-file-write.test
@@ -1,6 +1,7 @@
## Test failing to write output file on non-windows platforms.
# UNSUPPORTED: system-windows
+# REQUIRES: non-root-user
# RUN: rm -rf %t.TestDir
# RUN: mkdir %t.TestDir
# RUN: touch %t.TestDir/Output.TestFile
diff --git a/llvm/test/tools/llvm-ranlib/error-opening-permission.test b/llvm/test/tools/llvm-ranlib/error-opening-permission.test
index 1b1bb0def78d7..be56962112e6b 100644
--- a/llvm/test/tools/llvm-ranlib/error-opening-permission.test
+++ b/llvm/test/tools/llvm-ranlib/error-opening-permission.test
@@ -1,5 +1,6 @@
## Unsupported on windows as marking files "unreadable" is non-trivial on windows.
# UNSUPPORTED: system-windows
+# REQUIRES: non-root-user
# RUN: rm -rf %t && split-file %s %t && cd %t
# RUN: yaml2obj 1.yaml -o 1.o
diff --git a/llvm/utils/lit/lit/llvm/config.py b/llvm/utils/lit/lit/llvm/config.py
index 79094b839e772..d87d0bf92cd9d 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -5,6 +5,7 @@
import subprocess
import sys
import errno
+import getpass
import lit.util
from lit.llvm.subst import FindTool
@@ -12,6 +13,17 @@
lit_path_displayed = False
+def user_is_root():
+ # getpass.getuser() can throw an exception in some cases:
+ # See https://github.com/python/cpython/issues/76912
+ try:
+ if getpass.getuser() == 'root':
+ return True
+ except:
+ pass
+
+ return False
+
class LLVMConfig(object):
def __init__(self, lit_config, config):
@@ -154,6 +166,9 @@ def __init__(self, lit_config, config):
if re.match(r'^ppc64le.*-linux', target_triple):
features.add('target=powerpc64le-linux')
+ if not user_is_root():
+ features.add('non-root-user')
+
use_gmalloc = lit_config.params.get("use_gmalloc", None)
if lit.util.pythonize_bool(use_gmalloc):
# Allow use of an explicit path for gmalloc library.
|
|
✅ With the latest revision this PR passed the Python code formatter. |
There was a problem hiding this comment.
LGTM. Please wait for @MaskRay / others to chime in though.
Thanks for putting this together. Given the CI constraints, I think there's a really compelling case for adding this in.
|
How about testing for the ACL bypassing behaviour instead of checking for username == root? $ sudo python3 -c '
import tempfile
from pathlib import Path
with tempfile.TemporaryDirectory() as td:
Path(td, "test.txt").write_text("test")
Path(td, "test.txt").chmod(0o100)
print(Path(td, "test.txt").read_text())
'
test
python3 -c '
import tempfile
from pathlib import Path
with tempfile.TemporaryDirectory() as td:
Path(td, "test.txt").write_text("test")
Path(td, "test.txt").chmod(0o100)
print(Path(td, "test.txt").read_text())
'
Traceback (most recent call last):
File "<string>", line 8, in <module>
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/pathlib.py", line 1256, in read_text
with self.open(mode='r', encoding=encoding, errors=errors) as f:
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/pathlib.py", line 1242, in open
return io.open(self, mode, buffering, encoding, errors, newline,
File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/pathlib.py", line 1110, in _opener
return self._accessor.open(self, flags, mode)
PermissionError: [Errno 13] Permission denied: '/var/folders/_5/ltg5zrm5379_yh38z_j2vvgr00w_dz/T/tmpu5g01tt8/test.txt' |
llvm/utils/lit/lit/llvm/config.py
Outdated
| # getpass.getuser() can throw an exception in some cases: | ||
| # See https://github.com/python/cpython/issues/76912 | ||
| try: | ||
| if getpass.getuser() == "root": |
There was a problem hiding this comment.
*NIX systems can use os.getuid == 0
|
Any thoughts on using os.getuid() vs an explicit test for the ACL bypassing behavior? |
@arichardson How portable would this check be? Are there any scenarios where this check would return a different result than checking username == root? |
binary with setuid bit will have getuser() return the user that ran the binary but the binary will behave as root. |
But we are checking for the permissions that the python interpreter + spawned binaries have, so setuid binaries don't matter. If llvm-ar was setuid we would have a whole new set of problems regardless of current user privileges. I don't feel strongly which check is used I just thought checking whether the behaviour that makes the test fail is present is slightly cleaner than doing so indirectly via getuid==0. The main problem I can think of with the ACL bypassing check is that Windows might behave weirdly but the tests are already disabled on Windows. |
All we need is for lit to be setuid for the check to return false despite everything being run with root privilege, right? And yes it might lead to privilege issue but so does running lit as root. That said I'm not arguing for a change, I didn't make the ACL comment. As pointed out this is quite useful for running things in containers and if someone goes to the trouble of having the container have a non root user but still use setuid it's looking for trouble. Not sure about ACL but I would assume it's similar: bad idea outside of a container because lit can run anything, all one need is create a test file with the right RUN line. In short, I'm personally happy with the patch as it is. My 2 cents. |
|
Thanks for the feedback. I would prefer to keep the patch as-is. The reason I don't want to use os.getuid() is that it is only available on Unix systems, and I don't want to have to add extra code to do OS detection. I also don't want to do a specific ACL test, because creating temporary files is adding another point of failure to lit start up. Since this check is only needed by 4 tests, I want to make it as simple as possible and reduce the risk that it will impact users running as non-root, which is the most common case. |
That sounds fine to me. I think using os.getuid instead of getpass is still possible since it's already wrapped inside a bare try-except so that should be safe and avoids an extra import? |
I tested this and it works, so I've updated the patch to use os.getuid now. |
There are a few test that check access permissions, so they need to be disabled when running the tests as root.
The most common use case for running tests as root is inside of a container. GitHub Actions, for example, only supports running the root user inside of containers, so this change is necessary in order to run the tests inside of a container running in the GitHub Actions environment.