Avoid panics when failing to parse zip files

This does require the removal of the From implementation; it couldn't be
converted to TryFrom due to lack of generic specialization
(rust-lang/rust#50133).

Signed-off-by: Ryan Gonzalez <ryan.gonzalez@collabora.com>

Author: refi64

gitlab-runner/src/artifact.rs

@@ -4,7 +4,7 @@ use std::{
     path::Path,
 };
 
-use zip::read::ZipFile;
+use zip::{read::ZipFile, result::ZipResult};
 
 /// A file in a gitlab artifact
 ///
@@ -35,15 +35,6 @@ impl Read for ArtifactFile<'_> {
 pub(crate) trait ReadSeek: Read + Seek {}
 impl<T> ReadSeek for T where T: Read + Seek {}
 
-impl<T> From<T> for Artifact
-where
-    T: Send + Read + Seek + 'static,
-{
-    fn from(data: T) -> Self {
-        Artifact::new(Box::new(data))
-    }
-}
-
 /// An artifact downloaded from gitlab
 ///
 /// The artifact holds a set of files which can be read out one by one
@@ -52,10 +43,9 @@ pub struct Artifact {
 }
 
 impl Artifact {
-    pub(crate) fn new(data: Box<dyn ReadSeek + Send>) -> Self {
-        //let reader = std::io::Cursor::new(data);
-        let zip = zip::ZipArchive::new(data).unwrap();
-        Self { zip }
+    pub(crate) fn new(data: Box<dyn ReadSeek + Send>) -> ZipResult<Self> {
+        let zip = zip::ZipArchive::new(data)?;
+        Ok(Self { zip })
     }
 
     /// Iterator of the files names inside the artifacts

gitlab-runner/src/client.rs

@@ -8,6 +8,7 @@ use std::time::Duration;
 use thiserror::Error;
 use tokio::io::{AsyncWrite, AsyncWriteExt};
 use url::Url;
+use zip::result::ZipError;
 
 fn deserialize_null_default<'de, D, T>(deserializer: D) -> Result<T, D::Error>
 where
@@ -196,6 +197,8 @@ pub enum Error {
     Request(#[from] reqwest::Error),
     #[error("Failed to write to destination {0}")]
     WriteFailure(#[source] futures::io::Error),
+    #[error("Failed to parse zip file: {0}")]
+    ZipFile(#[from] ZipError),
     #[error("Empty trace")]
     EmptyTrace,
 }

gitlab-runner/src/job.rs

@@ -200,13 +200,15 @@ impl ArtifactCache {
     async fn get(&self, id: u64) -> Result<Option<Artifact>, ClientError> {
         if let Some(data) = self.lookup(id) {
             match data {
-                CacheData::MemoryBacked(m) => Ok(Some(Artifact::from(std::io::Cursor::new(m)))),
+                CacheData::MemoryBacked(m) => {
+                    Ok(Some(Artifact::new(Box::new(std::io::Cursor::new(m)))?))
+                }
                 CacheData::FileBacked(p) => {
                     let f = tokio::fs::File::open(p)
                         .await
                         .map_err(ClientError::WriteFailure)?;
                     // Always succeeds as no operations have been started
-                    Ok(Some(Artifact::from(f.try_into_std().unwrap())))
+                    Ok(Some(Artifact::new(Box::new(f.try_into_std().unwrap()))?))
                 }
             }
         } else {