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.

Sign up for GitHub

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

Jump to bottom

Don't mask rpm2cpio failure in Pkg._extract_rpm() #1266

Merged
merged 6 commits into from
Sep 6, 2024
Merged

Don't mask rpm2cpio failure in Pkg._extract_rpm() #1266

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
42cdb0d
Don't mask rpm2cpio failure in Pkg._extract_rpm()
dmach Aug 12, 2024
088ced3
Remove test_directory_without_x_permission2 test
dmach Aug 14, 2024
c404f3d
Exclude python311-pytest-xprocess from test_lint because rpm2cpio fai…
dmach Aug 14, 2024
ffe4b2c
Rewrite test_extract to *_fail variant that tests an expected failure…
dmach Aug 14, 2024
cd5f890
Merge branch 'main' into fix-rpm2cpio
danigm Sep 4, 2024
ce08677
test: Fix test_extract_fail when running as root
danigm Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rpmlint/pkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

This f-string which depends on
library input
Loading
is later used in a
shell command
Loading
.
This f-string which depends on
library input
Loading
is later used in a
shell command
Loading
.
This f-string which depends on
library input
Loading
is later used in a
shell command
Loading
.
This f-string which depends on
library input
Loading
is later used in a
shell command
Loading
.
This f-string which depends on
library input
Loading
is later used in a
shell command
Loading
.
This f-string which depends on
library input
Loading
is later used in a
shell command
Loading
.
This f-string which depends on
library input
Loading
is later used in a
shell command
Loading
.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dmach dmach Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

subprocess.check_output(command_str, shell=True, env=ENGLISH_ENVIRONMENT, stderr=stderr)
self.extracted = True
return dirname
Expand Down
Loading