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

live: clean up CPIO handling; support extracting from arbitrarily concatenated archives #708

Merged
merged 11 commits into from
Dec 14, 2021

Conversation

bgilbert
Copy link
Contributor

@bgilbert bgilbert commented Dec 9, 2021

  1. The CPIO code was written pretty early and could use some cleanup.
  2. Teach coreos-installer to extract files from arbitrarily concatenated CPIO archives, such as an Ignition config that's been concatenated onto the official OS initrd. This only affects pxe ignition unwrap for now, but will also be useful infrastructure for switching pxe customize to producing a concatenated initrd. That's more consistent with the iso customize UX, and also allows us to avoid possible version skew between the generated config and the OS version it's intended for.

We were letting xz2 finalize compression in Drop and not checking the
return value.
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Cool stuff, LGTM overall! A bit unfortunate about the xz2 copying. Would be good to file something upstream and link it from the commit message and file header at least.

Last commit message should also expand on the why.

src/io/xz.rs Outdated Show resolved Hide resolved
src/io/initrd.rs Outdated Show resolved Hide resolved
src/io/initrd.rs Outdated Show resolved Hide resolved
src/io/initrd.rs Outdated Show resolved Hide resolved
Have the extraction function return None if the member is not found,
rather than erroring out.
If the buffer passed to read() is zero-length, a return value of Ok(0)
does not indicate EOF.  Don't do a trailing-garbage check in this case.

I'm not aware of this bug occurring in the wild, and current code paths
shouldn't exercise it.
@bgilbert
Copy link
Contributor Author

It turns out xz2::write::XzDecoder provides the semantics we need for trailing data, so I've dropped the copied code and added an adapter that implements an xz2::bufread::XzDecoder-like API on top of xz2::write::XzDecoder. I submitted alexcrichton/xz2-rs#86 upstream, so hopefully we can remove the adapter code eventually. I've referenced that PR from a comment, but omitted it from the commit message to avoid spamming the PR.

Dropped the last commit for now. It's better suited to the followup PR where it'll be used.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM!

@bgilbert bgilbert enabled auto-merge December 13, 2021 17:24
@bgilbert bgilbert disabled auto-merge December 14, 2021 03:04
Fix mishandling of two cases:

- Read can return Ok(0) when not at EOF if the buffer argument is
zero-length.

- It's legal to call read() again after EOF, in which case we were trying
to unwrap the thread handle again and panicking.  Instead, return any
thread error when we first reach EOF, and Ok(0) afterward.
We're about to switch to an xz implementation that doesn't handle trailing
garbage detection itself.
xz2::bufread::XzDecoder errors on trailing garbage in the input stream,
but we need to support trailing data to decode arbitrary initrds.
xz2::write::XzDecoder does not error in this case, so implement the API
of bufread::XzDecoder in terms of write::XzDecoder.
This allows `pxe ignition unwrap` to extract Ignition configs from initrds
that have been concatenated to the OS initrd artifact.  It will also be
useful for other things later.
The kernel uses the last copy of a file in the archive, not the first,
so we should do the same.
@bgilbert
Copy link
Contributor Author

CI caught a failure 🎈 🎉 🎆 in osmet unpacking because we were calling read() twice at EOF of uncompressed streams. Fixed the double read() in DecompressReader (it's unnecessary) and fixed EOF handling in OsmetUnpacker.

@bgilbert bgilbert merged commit 25a780b into coreos:main Dec 14, 2021
@bgilbert bgilbert deleted the cpio branch December 14, 2021 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants