[pbs-devel] applied: [PATCH pxar] fix `decode_entry` on special files

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

thanks for testing, applied

On Tue, Dec 15, 2020 at 12:08:53PM +0100, Dominik Csapak wrote:
> looks good AFAICT
> fixes the problem with fifo/sockets
> 
> Tested-By: Dominik Csapak <d.csapak@proxmox.com>
> 
> On 12/15/20 11:37 AM, Wolfgang Bumiller wrote:
> > When using the random accessor to access FIFOs or sockets,
> > the ranged reader limits the data to only that entry, and
> > the `decode_entry` will never see a `PAYLOAD` or
> > `GOODBYE_TABLE` item to finish the entry.
> > Instead, it'll reach EOF and we need to handle this.
> > The accessor now tells the decoder to expect EOF as a valid
> > condition for ending the entry.
> > 
> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> > ---
> > 
> > The included testcase previously failed with
> >      "failed to decode entry for fifo0"
> > 
> >   src/accessor/mod.rs  |  2 +-
> >   src/decoder/mod.rs   | 52 +++++++++++++++++++++++++++++++++++---------
> >   tests/simple/main.rs | 46 +++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 89 insertions(+), 11 deletions(-)
> > 
> > diff --git a/src/accessor/mod.rs b/src/accessor/mod.rs
> > index 0ab03a6..d02dc13 100644
> > --- a/src/accessor/mod.rs
> > +++ b/src/accessor/mod.rs
> > @@ -229,7 +229,7 @@ async fn get_decoder<T: ReadAt>(
> >       entry_range: Range<u64>,
> >       path: PathBuf,
> >   ) -> io::Result<DecoderImpl<SeqReadAtAdapter<T>>> {
> > -    Ok(DecoderImpl::new_full(SeqReadAtAdapter::new(input, entry_range), path).await?)
> > +    Ok(DecoderImpl::new_full(SeqReadAtAdapter::new(input, entry_range), path, true).await?)
> >   }
> >   // NOTE: This performs the Decoder::read_next_item() behavior! Keep in mind when changing!
> > diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs
> > index fcc2dd9..2a5e79a 100644
> > --- a/src/decoder/mod.rs
> > +++ b/src/decoder/mod.rs
> > @@ -155,6 +155,10 @@ pub(crate) struct DecoderImpl<T> {
> >       path_lengths: Vec<usize>,
> >       state: State,
> >       with_goodbye_tables: bool,
> > +
> > +    /// The random access code uses decoders for sub-ranges which may not end in a `PAYLOAD` for
> > +    /// entries like FIFOs or sockets, so there we explicitly allow an item to terminate with EOF.
> > +    eof_after_entry: bool,
> >   }
> >   enum State {
> > @@ -191,14 +195,18 @@ pub(crate) enum ItemResult {
> >   impl<I: SeqRead> DecoderImpl<I> {
> >       pub async fn new(input: I) -> io::Result<Self> {
> > -        Self::new_full(input, "/".into()).await
> > +        Self::new_full(input, "/".into(), false).await
> >       }
> >       pub(crate) fn input(&self) -> &I {
> >           &self.input
> >       }
> > -    pub(crate) async fn new_full(input: I, path: PathBuf) -> io::Result<Self> {
> > +    pub(crate) async fn new_full(
> > +        input: I,
> > +        path: PathBuf,
> > +        eof_after_entry: bool,
> > +    ) -> io::Result<Self> {
> >           let this = DecoderImpl {
> >               input,
> >               current_header: unsafe { mem::zeroed() },
> > @@ -210,6 +218,7 @@ impl<I: SeqRead> DecoderImpl<I> {
> >               path_lengths: Vec::new(),
> >               state: State::Begin,
> >               with_goodbye_tables: false,
> > +            eof_after_entry,
> >           };
> >           // this.read_next_entry().await?;
> > @@ -383,7 +392,14 @@ impl<I: SeqRead> DecoderImpl<I> {
> >               self.current_header = unsafe { mem::zeroed() };
> > -            while self.read_next_item().await? != ItemResult::Entry {}
> > +            loop {
> > +                match self.read_next_item_or_eof().await? {
> > +                    Some(ItemResult::Entry) => break,
> > +                    Some(ItemResult::Attribute) => continue,
> > +                    None if self.eof_after_entry => break,
> > +                    None => io_bail!("unexpected EOF in entry"),
> > +                }
> > +            }
> >               if self.entry.is_dir() {
> >                   self.path_lengths
> > @@ -402,24 +418,40 @@ impl<I: SeqRead> DecoderImpl<I> {
> >               .ok_or_else(|| io_format_err!("unexpected EOF"))
> >       }
> > +    async fn read_next_item(&mut self) -> io::Result<ItemResult> {
> > +        match self.read_next_item_or_eof().await? {
> > +            Some(item) => Ok(item),
> > +            None => io_bail!("unexpected EOF"),
> > +        }
> > +    }
> > +
> > +    // NOTE: The random accessor will decode FIFOs and Sockets in a decoder instance with a ranged
> > +    // reader so there is no PAYLOAD or GOODBYE TABLE to "end" an entry.
> > +    //
> >       // NOTE: This behavior method is also recreated in the accessor's `get_decoder_at_filename`
> >       // function! Keep in mind when changing!
> > -    async fn read_next_item(&mut self) -> io::Result<ItemResult> {
> > -        self.read_next_header().await?;
> > -        self.read_current_item().await
> > +    async fn read_next_item_or_eof(&mut self) -> io::Result<Option<ItemResult>> {
> > +        match self.read_next_header_or_eof().await? {
> > +            Some(()) => self.read_current_item().await.map(Some),
> > +            None => Ok(None),
> > +        }
> >       }
> > -    async fn read_next_header(&mut self) -> io::Result<()> {
> > +    async fn read_next_header_or_eof(&mut self) -> io::Result<Option<()>> {
> >           let dest = unsafe {
> >               std::slice::from_raw_parts_mut(
> >                   &mut self.current_header as *mut Header as *mut u8,
> >                   size_of_val(&self.current_header),
> >               )
> >           };
> > -        seq_read_exact(&mut self.input, dest).await?;
> > -        self.current_header.check_header_size()?;
> > -        Ok(())
> > +        match seq_read_exact_or_eof(&mut self.input, dest).await? {
> > +            Some(()) => {
> > +                self.current_header.check_header_size()?;
> > +                Ok(Some(()))
> > +            }
> > +            None => Ok(None),
> > +        }
> >       }
> >       /// Read the next item, the header is already loaded.
> > diff --git a/tests/simple/main.rs b/tests/simple/main.rs
> > index c73ca10..f15a0f5 100644
> > --- a/tests/simple/main.rs
> > +++ b/tests/simple/main.rs
> > @@ -42,6 +42,9 @@ fn test1() {
> >       assert!(!file.is_empty(), "encoder did not write any data");
> > +    // may be useful for testing...
> > +    // std::fs::write("myarchive.pxar", &file).expect("failed to write out test archive");
> > +
> >       let mut input = &file[..];
> >       let mut decoder = decoder::Decoder::from_std(&mut input).expect("failed to create decoder");
> >       let decoded_fs =
> > @@ -53,6 +56,7 @@ fn test1() {
> >           .expect("failed to create random access reader for encoded archive");
> >       check_bunzip2(&accessor);
> > +    check_run_special_files(&accessor);
> >   }
> >   fn check_bunzip2(accessor: &accessor::Accessor<&[u8]>) {
> > @@ -85,3 +89,45 @@ fn check_bunzip2(accessor: &accessor::Accessor<&[u8]>) {
> >       assert_eq!(content, "This is the bzip2 executable");
> >   }
> > +
> > +fn check_run_special_files(accessor: &accessor::Accessor<&[u8]>) {
> > +    let rundir = accessor
> > +        .open_root()
> > +        .expect("failed to open root of encoded archive")
> > +        .lookup("run")
> > +        .expect("failed to open /run in encoded archive")
> > +        .expect("missing /run in encoded archive")
> > +        .enter_directory()
> > +        .expect("expected /run to be a directory in the test archive");
> > +
> > +    assert_eq!(rundir.entry_count(), 2, "expected 2 entries in /run");
> > +
> > +    let mut rd = rundir.read_dir();
> > +    let fifo0 = rd
> > +        .next()
> > +        .expect("expected 'fifo0' entry in rundir")
> > +        .expect("failed to get first (fifo0) entry in test archive /run directory");
> > +    assert_eq!(
> > +        fifo0.file_name(),
> > +        Path::new("fifo0"),
> > +        "expected first file in /run to be fifo0"
> > +    );
> > +
> > +    let _entry = fifo0
> > +        .decode_entry()
> > +        .expect("failed to decode entry for fifo0");
> > +
> > +    let sock0 = rd
> > +        .next()
> > +        .expect("expected 'sock0' entry in rundir")
> > +        .expect("failed to get second (sock0) entry in test archive /run directory");
> > +    assert_eq!(
> > +        sock0.file_name(),
> > +        Path::new("sock0"),
> > +        "expected second file in /run to be sock0"
> > +    );
> > +
> > +    let _entry = sock0
> > +        .decode_entry()
> > +        .expect("failed to decode entry for sock0");
> > +}
> >