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

Added uncompress+file checks #12173

Merged
merged 1 commit into from
Jun 11, 2021
Merged

Conversation

rincebrain
Copy link
Contributor

@rincebrain rincebrain commented Jun 1, 2021

(Once this gets signoff from people as a reasonable idea, I'll go make a PR for the build docs and bb-dependencies.sh to add the "ncompress" package for RPM distro instructions. At this rate, I'll soon have just added * to the install command...)

Motivation and Context

Having an old enough version of "file" and no "uncompress" program installed can cause rpmbuild as root to crash and mangle rpmdb, including on latest CentOS 8. "Oops."

As a brief summary of places this does or doesn't occur:

  • 5.37 is the first release of upstream file that has this fixed (though, amusingly, it was not patched because of incorrectness, but for performance reasons)
  • Debian-alikes ship "uncompress" as a copy of "gunzip" (Debian hardlinks it; Ubuntu just copies it), so this never comes up even though buster has an old enough version (5.35) of file.
  • CentOS 7 has an ancient version of file (5.11), but this never happened in >50 runs when it usually takes 10 or fewer on vulnerable platforms; I'm going to conclude that it's because there was nontrivial churn and restructuring in the guilty function (uncompressbuf in compress.c)
  • CentOS 8 has a vulnerable version (5.33), and does crash and burn sometimes.
  • Fedora 29 has a vulnerable version (5.34) and crashes and burns
  • Fedora 30 had a vulnerable version (5.36) but shipped a patched version
  • Fedora 34 is too new (5.39)
  • FreeBSD never cares, since it's not calling rpmbuild

(See also: #12071 and #12168)

Description

Added checks for file's version and uncompress's existence, then something that checks if file's major version is 5 and minor is less than 37; if so, and uncompress is absent, error out if attempting to build any rpm targets.

I also added checks to the RPM specfiles to require ncompress (a package which provides an uncompress on RPM distros) if on an EL distro <= 8 or a Fedora distro <= 30, to catch the odd case where someone runs ./configure and then uninstalls uncompress.

(If/when CentOS 8 ships a patched version of file, this could be revised to instead require >= patched version of file for each of Fedora and EL distros, but A) that hasn't happened yet and B) I have no idea how long it takes patched versions to propagate among EL derivatives)

How Has This Been Tested?

Built rpm-{kmod,utils} on:

  • CentOS 7, with and without ncompress (it naturally refused with the latter)
  • CentOS 8, with and without ncompress (same)
  • Fedora 29, with and without ncompress (and again)
  • Fedora 34, with and without ncompress (both work)
  • Debian buster, with and without uncompress (the former works; the latter by renaming it out of the way; it fails)
  • Debian bullseye, with uncompress (works)

Also a non-RPM build on FreeBSD 14-CURRENT x86_64 worked fine.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@jwk404 jwk404 added the Status: Code Review Needed Ready for review and testing label Jun 1, 2021
@jwk404 jwk404 requested a review from ofaaland June 2, 2021 16:08
rincebrain added a commit to rincebrain/zfs-buildbot that referenced this pull request Jun 4, 2021
openzfs/zfs#12173 is going to add a requirement for ncompress.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
rincebrain added a commit to rincebrain/openzfs-docs that referenced this pull request Jun 4, 2021
openzfs/zfs#12173 is going to add a requirement for ncompress.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
gmelikov pushed a commit to openzfs/openzfs-docs that referenced this pull request Jun 4, 2021
openzfs/zfs#12173 is going to add a requirement for ncompress.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
behlendorf pushed a commit to openzfs/zfs-buildbot that referenced this pull request Jun 4, 2021
openzfs/zfs#12173 is going to add a requirement for ncompress.

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@behlendorf
Copy link
Contributor

I've gone ahead and merged the buildbot change. If you can force update the PR we can verify all is well.

@rincebrain
Copy link
Contributor Author

rincebrain commented Jun 4, 2021

We'll see if this gets any traction, thanks.

Pushed.

Huh. I'll be kind of impressed if the sanity test failures are related, but I'll look at them.

edit: It looks like it's just a chain after a zfs destroy -r testpool/testfs1 got back EBUSY, which I'd be pretty impressed if this can cause.

@behlendorf
Copy link
Contributor

Huh. I'll be kind of impressed if the sanity test failures are related, but I'll look at them.

Indeed! Lucky you, another rare glitch which occasionally pops up in the CI and nowhere else! But totally unrelated.

@rincebrain
Copy link
Contributor Author

Yeah, at least whatever's happening to the FBSD 13 bot happens > 90% of the time (though I still haven't been able to reproduce it...it'd probably be simpler to open an AWS account and use the exact AMI.)

@behlendorf
Copy link
Contributor

Speaking of the FreeBSD fixes, they've just been merged. So if you're game to rebase this one last time there's a chance all of the bots might pass. Though I don't think it's needed given the scope of this change.

Having an old enough version of "file" and no "uncompress" program
installed can cause rpmbuild as root to crash and mangle rpmdb.

So let's add a build dependency for RPM-based systems.

Closes: openzfs#12071
Closes: openzfs#12168

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 10, 2021
@jwk404 jwk404 merged commit 1a345d6 into openzfs:master Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants