Don't mask rpm2cpio failure in Pkg._extract_rpm() #1266
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -610,7 +610,7 @@ | |||
| subprocess.check_output('rpm2archive - | tar -xz && chmod -R +rX .', shell=True, env=ENGLISH_ENVIRONMENT, | |||
| stderr=stderr, stdin=rpm_data) | |||
| else: | |||
| command_str = f'rpm2cpio {quote(str(filename))} | cpio -id ; chmod -R +rX .' | |||
| command_str = f'rpm2cpio {quote(str(filename))} | cpio -id && chmod -R +rX .' | |||
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
| @@ -610,7 +610,7 @@ def _extract_rpm(self, dirname, verbose): | |||
| subprocess.check_output('rpm2archive - | tar -xz && chmod -R +rX .', shell=True, env=ENGLISH_ENVIRONMENT, | |||
| stderr=stderr, stdin=rpm_data) | |||
| else: | |||
| command_str = f'rpm2cpio {quote(str(filename))} | cpio -id ; chmod -R +rX .' | |||
| command_str = f'rpm2cpio {quote(str(filename))} | cpio -id && chmod -R +rX .' | |||
There was a problem hiding this comment.
This makes the tests fails on tumbleweed:
cpio: ./etc/raddb/mods-config/sql/moonshot-targeted-ids/mysql/queries.conf: Cannot open: Permission denied
cpio: ./etc/raddb/mods-config/sql/moonshot-targeted-ids/mysql/schema.sql: Cannot open: Permission denied
cpio: ./etc/raddb/mods-config/sql/moonshot-targeted-ids/postgresql/queries.conf: Cannot open: Permission denied
cpio: ./etc/raddb/mods-config/sql/moonshot-targeted-ids/postgresql/schema.sql: Cannot open: Permission denied
cpio: ./etc/raddb/mods-config/sql/moonshot-targeted-ids/sqlite/queries.conf: Cannot open: Permission denied
cpio: ./etc/raddb/mods-config/sql/moonshot-targeted-ids/sqlite/schema.sql: Cannot open: Permission denied
7381 blocks
(none): E: fatal error while reading test/binary/freeradius-server-3.2.3-3.1.x86_64.rpm: Command 'rpm2cpio /builddir/build/BUILD/rpmlint-2.6.1/test/binary/freeradius-server-3.2.3-3.1.x86_64.rpm | cpio -id && chmod -R +rX .' returned non-zero exit status 2.
This is because cpio -id can't extract the tests packages completely right now because of permissions, so we need to fix the problem before using &&. With the ; this commands works because the chmod command is working.
It's true that this is hiding the real problem, but it could be good to fix the tests at the same time we do this change. I need to investigate a bit more about it.
These are the broken test packages right now:
FAILED test/test_files.py::test_directory_without_x_permission2[binary/freeradius-server] - subprocess.CalledProcessError: Command 'rpm2cpio /builddir/build/BUILD/rpmlint-2.6.1/test/binary/freeradius-server-3.2.3-3.1.x86_64.rpm | cpio -id && chmod -R +rX .' returned non-zero exit status 2.
FAILED test/test_lint.py::test_run_full_rpm[configs0-packages0] - subprocess.CalledProcessError: Command 'rpm2cpio /builddir/build/BUILD/rpmlint-2.6.1/test/binary/freeradius-server-3.2.3-3.1.x86_64.rpm | cpio -id && chmod -R +rX .' returned non-zero exit status 2.
FAILED test/test_pkg.py::test_extract[binary/python311-pytest-xprocess] - subprocess.CalledProcessError: Command 'rpm2cpio /builddir/build/BUILD/rpmlint-2.6.1/test/binary/python311-pytest-xprocess-0.23.0-2.4.noarch.rpm | cpio -id && chmod -R +rX .' returned non-zero exit status 2.
There was a problem hiding this comment.
I see. Any idea how to fix the problem on Tumbleweed?
Also, I forgot to add context about how I discovered this problem:
When a large srpm on Tumbleweed gets checked by rpmlint, rpm2cpio is used and errors out due to incompatible package payload with cpio. rpm2archive is not available on Tumbleweed. The problem was masked by the rpm2cpio always succeeding. I already submitted a change for building rpm2archive on Tumbleweed: https://build.opensuse.org/request/show/1193630
There was a problem hiding this comment.
I looked into the failures and it seems that cpio tries to extract a file to a directory that doesn't have the x flag set. Permission denied is the expected result:
$ mkdir -m 600 dir
$ touch dir/file
touch: cannot touch 'dir/file': Permission denied
I believe rpm workarounds this by setting file perms after they're all extracted.
I think all these test failures should be marked as expected failures.
Running rpmlint on partially extracted rpms may lead to incorrect results.
There was a problem hiding this comment.
Yes, that should be the expected result. So I think this is correct, but I want to fix those tests that are failing. Migrate to the FakePkg class usage and remove the rpm files or do something like that.
There was a problem hiding this comment.
Another option is just to skip these tests with pytest.skip decorator and left a comment to fix, so we can merge this and I can take a look to the broken tests in the future.
I'm working on the tests migration, with the help of GSoC intern, so it's something that I'll need to check soon.
There was a problem hiding this comment.
I pushed additional commits that modify the tests.
Please let me know if and how you like them.
EDIT: the last commit will definitely require some changes, mocking shutil.which doesn't work as I expected.
It errors out due to rpm2cpio failure and there's test_directory_without_x_permission that covers the non-standard-dir-perm check
|
I've merged with the current main branch and fixed the tests, so I think this can be merged now. I don't know if any tooling run the rpmlint as non root user but if that's the case it will fail to unpack, that as you said it's the right behavior. Maybe we can improve a bit the output, adding more information when this happens, but at this point I think this is an improvement. |
'rpm2archive - | tar -xz && chmod -R +rX .'does it right 3 lines above the fixed code.