Merge ~jslarraz/review-tools:add-doc-about-libmagic-warning into review-tools:master

[Jorge Sancho Larraz]

rt/c/base_container: make use of python-libmagic more eviden

[Alex Murray (alexmurray)]

LGTM, but can you please update the commit description to also describe how and why you are removing the 3 entries from magic_binary_file_descriptions as well? Thanks. A commit message more like

rt/c/base_container: use magic compat API

...

would perhaps make more sense here.

review: Needs Fixing

diff --git a/reviewtools/available.py b/reviewtools/available.py
index eafa366..5b0dd3c 100755
--- a/reviewtools/available.py
+++ b/reviewtools/available.py
@@ -19,6 +19,7 @@ import shutil
 import tempfile
 
 from reviewtools.common import (
+    error,
     debug,
     warn,
     open_file_write,
@@ -26,7 +27,6 @@ from reviewtools.common import (
     MKDTEMP_PREFIX,
     _add_error,  # make a class
     get_rock_manifest,
-    get_snap_manifest,
 )
 import reviewtools.email as email
 
@@ -37,6 +37,7 @@ from reviewtools.store import (
     get_faked_build_and_stage_packages,
 )
 from reviewtools.usn import read_usn_db
+from reviewtools.containers.snap_container import SnapContainer, ContainerException
 
 email_update_required_text = """A scan of this %s shows that it was built with packages from the Ubuntu
 archive that have since received security updates. """
@@ -484,7 +485,11 @@ def scan_store(
 def scan_snap(secnot_db_fn, snap_fn, with_cves=False, ignore_pockets=list()):
     """Scan snap for packages with security notices"""
     out = ""
-    (man, dpkg) = get_snap_manifest(snap_fn)
+    try:
+        snap = SnapContainer(snap_fn)
+        (man, dpkg) = snap.get_snap_manifest()
+    except ContainerException as e:
+        error(str(e))
     man = get_faked_build_and_stage_packages(man)
 
     # Use dpkg.list when the snap ships it in a spot we honor. This is limited
diff --git a/reviewtools/common.py b/reviewtools/common.py
index e2e80ff..d2c15b8 100644
--- a/reviewtools/common.py
+++ b/reviewtools/common.py
@@ -424,30 +424,6 @@ def cmdIgnoreErrorStrings(command, ignoreErrorStrings):
     return [rc, out]
 
 
-def _unpack_cmd(cmd_args, d, dest):
-    """Low level unpack helper"""
-    curdir = os.getcwd()
-    os.chdir(d)
-
-    (rc, out) = cmdIgnoreErrorStrings(cmd_args, UNSQUASHFS_IGNORED_ERRORS)
-
-    os.chdir(curdir)
-
-    if rc != 0:
-        if os.path.isdir(d):
-            recursive_rm(d)
-        error("unpacking failed with '%d':\n%s" % (rc, out))
-
-    if dest is None:
-        dest = d
-    else:
-        # Recursively move unpacked content to original dir, keeping
-        # permissions
-        move_dir_content(d, dest)
-
-    return dest
-
-
 # The original directory might have restrictive permissions, so save
 # them off, chmod the dir, do the move and reapply
 def move_dir_content(d, dest):
@@ -621,24 +597,6 @@ def unsquashfs_lln_parse(lln_out):
     return hdr, entries
 
 
-def _calculate_snap_unsquashfs_uncompressed_size(snap_pkg):
-    """Calculate size of the uncompressed snap"""
-    (rc, out) = cmd(["unsquashfs", "-lln", snap_pkg])
-    if rc != 0:
-        error("unsquashfs -lln '%s' failed: %s" % (snap_pkg, out))
-
-    size = 0
-    for line in out.splitlines():
-        if not line.startswith("-"):  # skip non-regular files
-            continue
-        try:
-            size += int(line.split()[2])
-        except ValueError:  # skip non-numbers
-            continue
-
-    return size
-
-
 def _calculate_rock_untar_uncompressed_size(rock_pkg):
     """Calculate size of the uncompressed tar"""
     size = 0
@@ -656,34 +614,6 @@ def unsquashfs_supports_ignore_errors():
     return "-ig[nore-errors]" in out
 
 
-def _unpack_snap_squashfs(snap_pkg, dest, items=[]):
-    """Unpack a squashfs based snap package to dest"""
-    size = _calculate_snap_unsquashfs_uncompressed_size(snap_pkg)
-
-    snap_max_size = MAX_UNCOMPRESSED_SIZE * 1024 * 1024 * 1024
-    valid_size, error_msg = is_pkg_uncompressed_size_valid(
-        snap_max_size, size, snap_pkg
-    )
-    if valid_size:
-        global MKDTEMP_PREFIX
-        global MKDTEMP_DIR
-        d = tempfile.mkdtemp(prefix=MKDTEMP_PREFIX, dir=MKDTEMP_DIR)
-        cmd = ["unsquashfs", "-no-progress", "-f", "-d", d]
-
-        # If unsquashfs supports it, pass "-ignore-errors -quiet"
-        if unsquashfs_supports_ignore_errors():
-            cmd.append("-ignore-errors")
-            cmd.append("-quiet")
-
-        cmd.append(os.path.abspath(snap_pkg))
-
-        if len(items) != 0:
-            cmd += items
-        return _unpack_cmd(cmd, d, dest)
-    else:
-        error(error_msg)
-
-
 def is_pkg_uncompressed_size_valid(pkg_max_size, size, pkg):
     st = os.statvfs(pkg)
     avail = st.f_bsize * st.f_bavail * 0.9  # 90% of available space
@@ -751,21 +681,6 @@ def _unpack_rock_tar(rock_pkg, dest):
         error(error_msg)
 
 
-def unpack_pkg(fn, dest=None, items=[]):
-    """Unpack package"""
-    pkg = check_fn(fn)
-    check_dir(dest)
-
-    # Limit the maximimum size of the package
-    check_max_pkg_size(pkg)
-
-    # check if its a squashfs based snap
-    if is_squashfs(pkg):
-        return _unpack_snap_squashfs(fn, dest, items)
-
-    error("Unsupported package format (not squashfs)")
-
-
 def check_dir(dest):
     if dest is not None and os.path.exists(dest):
         error("'%s' exists. Aborting." % dest)
@@ -790,13 +705,6 @@ def check_max_pkg_size(pkg):
         )
 
 
-def is_squashfs(filename):
-    """Return true if the given filename as a squashfs header"""
-    with open(filename, "rb") as f:
-        header = f.read(10)
-    return header.startswith(b"hsqs")
-
-
 def unpack_rock(fn, dest=None):
     """Unpack rock"""
     pkg = check_fn(fn)
@@ -1078,56 +986,6 @@ def read_file_as_json_dict(fn):
     return raw
 
 
-def get_snap_manifest(fn):
-    if "SNAP_USER_COMMON" in os.environ and os.path.exists(
-        os.environ["SNAP_USER_COMMON"]
-    ):
-        MKDTEMP_DIR = os.environ["SNAP_USER_COMMON"]
-    else:
-        MKDTEMP_DIR = tempfile.gettempdir()
-
-    man = "snap/manifest.yaml"
-    os_dpkg = "usr/share/snappy/dpkg.list"
-    snap_dpkg = "snap/dpkg.list"
-    # unpack_pkg() fails if this exists, so this is safe
-    dir = tempfile.mktemp(prefix=MKDTEMP_PREFIX, dir=MKDTEMP_DIR)
-    unpack_pkg(fn, dir, [man, os_dpkg, snap_dpkg])
-
-    man_fn = os.path.join(dir, man)
-    if not os.path.isfile(man_fn):
-        recursive_rm(dir)
-        error("%s not in %s" % (man, fn))
-
-    with open_file_read(man_fn) as fd:
-        try:
-            man_yaml = yaml.safe_load(fd)
-        except Exception:
-            recursive_rm(dir)
-            error("Could not load %s. Is it properly formatted?" % man)
-
-    os_dpkg_fn = os.path.join(dir, os_dpkg)
-    snap_dpkg_fn = os.path.join(dir, snap_dpkg)
-    dpkg_list = None
-    if os.path.isfile(os_dpkg_fn):
-        with open_file_read(os_dpkg_fn) as fd:
-            try:
-                dpkg_list = fd.readlines()
-            except Exception:
-                recursive_rm(dir)
-                error("Could not load %s. Is it properly formatted?" % os_dpkg)
-    elif os.path.isfile(snap_dpkg_fn):
-        with open_file_read(snap_dpkg_fn) as fd:
-            try:
-                dpkg_list = fd.readlines()
-            except Exception:
-                recursive_rm(dir)
-                error("Could not load %s. Is it properly formatted?" % snap_dpkg)
-
-    recursive_rm(dir)
-
-    return (man_yaml, dpkg_list)
-
-
 def get_rock_manifest(fn):
     if "SNAP_USER_COMMON" in os.environ and os.path.exists(
         os.environ["SNAP_USER_COMMON"]
diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py
index 9e6f104..0d51021 100644
--- a/reviewtools/containers/base_container.py
+++ b/reviewtools/containers/base_container.py
@@ -7,12 +7,19 @@ from tempfile import mkdtemp
 from reviewtools.common import (
     MKDTEMP_PREFIX,
     MKDTEMP_DIR,
-    debug,
-    unpack_pkg,
     recursive_rm,
 )
 
 
+# Add base_container private attributes to documentation
+__pdoc__ = {
+    "BaseContainer._min_packed_size": True,
+    "BaseContainer._max_packed_size": True,
+    "BaseContainer._min_unpacked_size": True,
+    "BaseContainer._max_unpacked_size": True,
+}
+
+
 class ContainerException(Exception):
     """This class represents exceptions"""
 
@@ -32,16 +39,36 @@ class BaseContainer:
     """
 
     magic_binary_file_descriptions = [
-        "application/x-executable; charset=binary",
-        "application/x-sharedlib; charset=binary",
-        "application/x-object; charset=binary",
-        # 18.04 and higher doesn't show the charset
         "application/x-executable",
         "application/x-sharedlib",
         "application/x-object",
         "application/x-pie-executable",
     ]
 
+    _min_packed_size = None
+    """
+    Minimum size of the container file in bytes. It will be used to check the
+    container size during initialization.
+    """
+
+    _max_packed_size = None
+    """
+    Maximum size of the container file in bytes. It will be used to check the
+    container size during initialization.
+    """
+
+    _min_unpacked_size = None
+    """
+    Minimum size of the unpacked directory in bytes. It will be used to check the
+    container size during initialization.
+    """
+
+    _max_unpacked_size = None
+    """
+    Maximum size of the unpacked directory in bytes. It will be used to check the
+    container size during initialization.
+    """
+
     _unpack_dir = None
     _files = None
     _bin_files = None
@@ -59,6 +86,9 @@ class BaseContainer:
         else:
             raise ContainerException("Could not find '%s'" % fn)
 
+        # Check max/min size if defined
+        self.check_container_size()
+
     @property
     def unpack_dir(self) -> str:
         """
@@ -67,8 +97,7 @@ class BaseContainer:
         will be returned.
         """
         if self._unpack_dir is None:
-            self._unpack_dir = ".".join(self.filename.split(".")[:-1])
-            unpack_pkg(self.filename, dest=self._unpack_dir)
+            self._unpack_dir = self.unpack()
         return self._unpack_dir
 
     @property
@@ -104,6 +133,117 @@ class BaseContainer:
         if os.path.exists(self.tmp_dir):
             recursive_rm(self.tmp_dir)
 
+    def check_container_format(self):
+        """
+        Checks that the container file has the expected format.
+
+        Raises:
+            NotImplementedError: Calling this method will raise an exception as
+                format is specific to the container type. This method needs to be
+                implemented by the specific subclasses.
+        """
+        raise NotImplementedError("Must be implemented by subclasses")
+
+    def get_container_size(self) -> int:
+        """
+        Uses os.path.getsize() to return the size of the container file.
+
+        Retruns:
+            int: Size in bytes of the container file
+        """
+        return os.path.getsize(self.filename)
+
+    def check_container_size(self):
+        """
+        Checks that the container size is within the range specified by
+        min_packed_size and max_packed_size. If any of those values is not specified
+        (i.e. it is None), the respective check will be skipped.
+
+        Raises:
+            ContainerException: If container size is less than minSize or greater
+                than maxSize, a ContainerException will be raised.
+        """
+        size = self.get_container_size()
+        if self._min_packed_size and (size < self._min_packed_size):
+            raise ContainerException(
+                "container file is too small (%dM < %dM)"
+                % (size / 1024 / 1024, self._min_packed_size / 1024 / 1024)
+            )
+        if self._max_packed_size and (size > self._max_packed_size):
+            raise ContainerException(
+                "container file is too large (%dM > %dM)"
+                % (size / 1024 / 1024, self._max_packed_size / 1024 / 1024)
+            )
+
+    def calculate_unpacked_size(self) -> int:
+        """
+        Estimates the space that the unpacked directory will use without unpacking.
+
+        Returns:
+            int: Estimated size in bytes of the unpacked directory
+
+        Raises:
+            NotImplementedError: Calling this method will raise an exception as the
+                estimation method is specific to the container type. This method
+                needs to be implemented by the specific subclasses.
+        """
+        raise NotImplementedError("Must be implemented by subclasses")
+
+    def check_unpacked_size(self):
+        """
+        Checks that the container size is within the range specified by
+        min_unpacked_size and max_unpacked_size. If any of those values is not
+        specified (i.e. it is None), the respective check will be skipped.
+
+        Raises:
+            ContainerException: If unpacked size is less than minSize or greater
+                than maxSize, a ContainerException will be raised.
+        """
+        size = self.calculate_unpacked_size()
+        if self._min_unpacked_size and (size < self._min_unpacked_size):
+            raise ContainerException(
+                "unpacked directory is too small (%dM < %dM)"
+                % (size / 1024 / 1024, self._min_unpacked_size / 1024 / 1024)
+            )
+        if self._max_unpacked_size and (size > self._max_unpacked_size):
+            raise ContainerException(
+                "unpacked directory is too large (%dM > %dM)"
+                % (size / 1024 / 1024, self._max_unpacked_size / 1024 / 1024)
+            )
+
+    def check_disk_space(self):
+        """
+        Checks that there is sufficient disk space available for the container to be
+        unpacked, i.e. that it will not consume more than 90% of the available disk
+
+        Raises:
+            ContainerException: If estimated unpacked size exceed the 90% of the
+                available space, an exception will be raised.
+        """
+        size = self.calculate_unpacked_size()
+        st = os.statvfs(self.tmp_dir)
+        avail = st.f_bsize * st.f_bavail * 0.9  # 90% of available space
+        if size > avail * 0.9:
+            raise ContainerException(
+                "insufficient available disk space (%dM > %dM)"
+                % (size / 1024 / 1024, avail / 1024 / 1024)
+            )
+
+    def unpack(self) -> str:
+        """
+        Extracts the content of the container file and returns the absolute path to
+        the newly unpacked directory.
+
+        Returns:
+            str: absolute path to the newly created unpack dir
+
+        Raises:
+            NotImplementedError: Calling this method will raise an exception as the
+                estimation method is specific to the container type. This method
+                needs to be implemented by the specific subclasses.
+        """
+        raise NotImplementedError("Must be implemented by subclasses")
+
     def get_file(self, fn: str) -> Union[str, None]:
         """
         Reads and return the raw content of the specified file.
@@ -117,7 +257,7 @@ class BaseContainer:
 
         Raises:
             ContainerException: Calling this method will raise an exception if
-            the requested file is not found.
+                the requested file is not found.
         """
         if not os.path.exists(os.path.join(self.unpack_dir, fn)):
             raise ContainerException("Could not find '%s'" % fn)
@@ -156,15 +296,22 @@ class BaseContainer:
         Returns:
             list(str): list of files present in the container
         """
-        mime = magic.open(magic.MAGIC_MIME)
+        mime = magic.compat.open(magic.MAGIC_MIME)
         mime.load()
         bin_files = []
         for file in self.get_files_list(abs=True):
             try:
+                # There are two different magic libraries for python, python-magic
+                # and python-libmagic. We are installing python-magic (which is the
+                # only one distributed via apt). However, python-magic seems to hang
+                # when it hits a pipe file.
+                # python-magic also bundles the python-libmagic module and exposes
+                # it through the following compatibility API. Using this bundled
+                # python-libmagic module will generate a deprecation warning. See
+                # https://github.com/ahupp/python-magic/blob/master/magic/__init__.py#L458
                 res = mime.file(file)
-            except Exception:  # pragma: nocover
-                # workaround for zesty python3-magic
-                debug("could not detemine mime type of '%s'" % file)
+            except OSError:
+                # TODO: file does not exists. Only hit for external symlinks
                 continue
 
             if not abs:
diff --git a/reviewtools/containers/snap_container.py b/reviewtools/containers/snap_container.py
new file mode 100644
index 0000000..ff1ee57
--- /dev/null
+++ b/reviewtools/containers/snap_container.py
@@ -0,0 +1,44 @@
+import yaml
+from reviewtools.containers.squashfs_container import (
+    SquashfsContainer,
+    ContainerException,
+)
+
+
+class SnapContainer(SquashfsContainer):
+    # TODO: review-tools was not checking for it. We still have some snaps in
+    # "tests/" that doesn't fit this limit and will need to be regenerated before
+    # this check can be enforced.
+    # _min_packed_size = 16 * 1024
+    _max_packed_size = 5 * 1024 * 1024 * 1024
+    _max_unpacked_size = 25 * 1024 * 1024 * 1024
+
+    def get_snap_manifest(self):
+        man = "snap/manifest.yaml"
+        os_dpkg = "usr/share/snappy/dpkg.list"
+        snap_dpkg = "snap/dpkg.list"
+
+        if self._unpack_dir is None:
+            self.unpack(items=[man, os_dpkg, snap_dpkg])
+
+        man_content = self.get_file(man).decode()
+        try:
+            man_yaml = yaml.safe_load(man_content)
+        except Exception:
+            raise ContainerException(
+                "Could not load %s. Is it properly formatted?" % man
+            )
+
+        dpkg_list = None
+        try:
+            dpkg_list = self.get_file(os_dpkg).decode().split("\n")
+            return (man_yaml, dpkg_list)
+        except ContainerException:
+            pass
+
+        try:
+            dpkg_list = self.get_file(snap_dpkg).decode().split("\n")
+        except ContainerException:
+            pass
+
+        return (man_yaml, dpkg_list)
diff --git a/reviewtools/containers/squashfs_container.py b/reviewtools/containers/squashfs_container.py
new file mode 100644
index 0000000..631fe76
--- /dev/null
+++ b/reviewtools/containers/squashfs_container.py
@@ -0,0 +1,111 @@
+import os
+import shutil
+from reviewtools.common import cmd, cmdIgnoreErrorStrings, UNSQUASHFS_IGNORED_ERRORS
+from reviewtools.containers.base_container import BaseContainer, ContainerException
+
+
+class SquashfsContainer(BaseContainer):
+    """
+    A container is a file that encapsulates a file system. This class extends
+    BaseContainer to handle functionality that is specific to Squashfs containers.
+    """
+
+    def __init__(self, fn):
+        """
+        If SquashfsContainer is initialized with a container file, it will also
+        verify that the provided file is actually a squashfs, in addition to the
+        checks performed by the BaseContainer.
+        """
+        super().__init__(fn)
+
+        # Check is squash file
+        self.check_container_format()
+
+    def check_container_format(self):
+        """
+        Checks that the container file has the expected format.
+
+        Raises:
+            ContainerException: This method will raise an exception if the
+            container file is not a squashfs container.
+        """
+        with open(self.filename, "rb") as f:
+            header = f.read(10)
+        if not header.startswith(b"hsqs"):
+            raise ContainerException("Unsupported package format (not squashfs)")
+
+    def calculate_unpacked_size(self) -> int:
+        """
+        Estimates the space that the unpacked directory will use by adding the
+        individual files size reported by "unsquashfs -lln" command. It may not
+        be completely accurate, as this command does not consider that each folder
+        requires at least one complete block
+
+        Returns:
+            int: Estimated size in bytes of the unpacked directory
+        """
+        (rc, out) = cmd(["unsquashfs", "-lln", self.filename])
+        if rc != 0:
+            raise RuntimeError("unsquashfs -lln '%s' failed: %s" % (self.filename, out))
+
+        size = 0
+        for line in out.splitlines():
+            if not line.startswith("-"):  # skip non-regular files
+                continue
+            try:
+                size += int(line.split()[2])
+            except ValueError:  # skip non-numbers
+                continue
+
+        return size
+
+    def unpack(self, items: list = [], force: bool = False) -> str:
+        """
+        Extracts the content of the container file and returns the absolute path to
+        the newly unpacked directory.
+
+        Args:
+            force (bool, optional): If ``True``, the unpack_dir will be overwritten
+                if it already exists. Defaults to ``False``
+
+        Returns:
+            str: absolute path to the newly created unpack dir
+
+        Raises:
+            ContainerException: This method will raise an ContainerException if the
+                unpack_dir already exists and this method was called without the
+                force argument set to True
+            RuntimeError: This method will raise a RuntimeError under the execution
+                of the "unsquashfs" command returns with an error code other than 0.
+        """
+        if self._unpack_dir is not None and not force:
+            raise ContainerException("'%s' exists. Aborting." % self._unpack_dir)
+
+        # Check disk space and space available in disk
+        self.check_unpacked_size()
+        self.check_disk_space()
+
+        # Unpack the container
+        stage_dir = os.path.join(self.tmp_dir, "stage")
+        cmd = [
+            "unsquashfs",
+            "-no-progress",
+            "-ignore-errors",
+            "-quiet",
+            "-f",
+            "-d",
+            stage_dir,
+            self.filename,
+        ] + items
+        (rc, out) = cmdIgnoreErrorStrings(cmd, UNSQUASHFS_IGNORED_ERRORS)
+        if rc != 0:
+            if os.path.isdir(stage_dir):
+                shutil.rmtree(stage_dir)
+            raise RuntimeError("unpacking failed with '%d':\n%s" % (rc, out))
+
+        # Update on success
+        if self._unpack_dir is not None and os.path.exists(self._unpack_dir):
+            shutil.rmtree(self._unpack_dir)
+        self._unpack_dir = ".".join(self.filename.split(".")[:-1])
+        shutil.move(stage_dir, self._unpack_dir)
+        return self._unpack_dir
diff --git a/reviewtools/sr_common.py b/reviewtools/sr_common.py
index 16790cc..9b961e2 100644
--- a/reviewtools/sr_common.py
+++ b/reviewtools/sr_common.py
@@ -30,7 +30,7 @@ from reviewtools.common import (
     unsquashfs_lln_parse,
     verify_type,
 )
-from reviewtools.containers.base_container import BaseContainer, ContainerException
+from reviewtools.containers.snap_container import SnapContainer, ContainerException
 from reviewtools.overrides import interfaces_attribs_addons
 
 
@@ -398,7 +398,7 @@ class SnapReview(ReviewBase):
         ReviewBase.__init__(self, review_type, overrides=overrides)
 
         try:
-            self.snap = BaseContainer(fn)
+            self.snap = SnapContainer(fn)
         except ContainerException as e:
             error(str(e))
         # TODO: update code to directly use self.snap.xxx instead of assigning here
diff --git a/reviewtools/sr_tests.py b/reviewtools/sr_tests.py
index 6caae54..bfbf375 100644
--- a/reviewtools/sr_tests.py
+++ b/reviewtools/sr_tests.py
@@ -87,7 +87,7 @@ def create_patches():
     # child.
     patches = []
     patches.append(
-        patch("reviewtools.sr_common.BaseContainer.__init__", _BaseContainer.__init__)
+        patch("reviewtools.sr_common.SnapContainer.__init__", _BaseContainer.__init__)
     )
     patches.append(
         patch("reviewtools.sr_common.SnapReview._extract_snap_yaml", _extract_snap_yaml)
@@ -98,7 +98,6 @@ def create_patches():
             _extract_snap_manifest_yaml,
         )
     )
-    patches.append(patch("reviewtools.common.unpack_pkg", _mock_func))
     # sr_common
     patches.append(
         patch("reviewtools.sr_common.SnapReview._get_unpack_dir", __get_unpack_dir)
diff --git a/reviewtools/tests/containers/test_base_container.py b/reviewtools/tests/containers/test_base_container.py
index 740e942..1b4e022 100644
--- a/reviewtools/tests/containers/test_base_container.py
+++ b/reviewtools/tests/containers/test_base_container.py
@@ -3,29 +3,43 @@ import shutil
 import unittest
 from tempfile import mkdtemp
 from reviewtools.containers.base_container import BaseContainer, ContainerException
-from reviewtools.tests.utils import make_snap2
 
 
 class TestBaseContainer(unittest.TestCase):
     """Tests for reviewtools.containers.base_container.BaseContainer"""
 
-    expected_files = ["meta/snap.yaml", "meta/icon.png", "nonexecstack.bin"]
-    expected_binaries = ["nonexecstack.bin"]
+    expected_files = ["meta/snap.yaml", "meta/icon.png", "bin/ls"]
+    expected_binaries = ["bin/ls"]
 
     @classmethod
     def setUpClass(cls):
         cls.tmp_dir = mkdtemp()
-        cls.fn = make_snap2(
-            output_dir=cls.tmp_dir,
-            yaml="Test",
-            extra_files=["/bin/ls:nonexecstack.bin"],
-        )
+        cls.fn = cls._create_container(cls)
+        cls.unpack_dir = cls._create_unpackdir(cls)
 
     @classmethod
     def tearDownClass(cls):
         if os.path.exists(cls.tmp_dir):
             shutil.rmtree(cls.tmp_dir)
 
+    def _create_container(self, container_size: int = 10000):
+        fn = os.path.join(self.tmp_dir, "container.test")
+        with open(fn, "w") as f:
+            f.truncate(container_size)
+        return fn
+
+    def _create_unpackdir(self):
+        unpack_dir = os.path.join(self.tmp_dir, "unpack_dir")
+        for file in self.expected_files:
+            unpacked_file = os.path.join(unpack_dir, file)
+            os.makedirs(os.path.dirname(unpacked_file), exist_ok=True)
+            if file not in self.expected_binaries:
+                with open(unpacked_file, "w+b") as fd:
+                    fd.write(b"Test")
+            else:
+                shutil.copyfile(os.path.join("/", file), unpacked_file)
+        return unpack_dir
+
     # Test class initialization
     def test_initialization_container__happy(self):
         try:
@@ -43,10 +57,75 @@ class TestBaseContainer(unittest.TestCase):
                 in context.exception.value
             )
 
+    # Test deletion
+    def test_check_cleanup(self):
+        try:
+            container = BaseContainer(self.fn)
+            tmp_dir = container.tmp_dir
+            del container
+            self.assertFalse(os.path.exists(tmp_dir))
+        except Exception:
+            self.fail("An unexpected error occurred during deletion test")
+
+    # Test check container size
+    def test_check_container_size__happy(self):
+        container = BaseContainer(self.fn)
+        container._min_packed_size = 0
+        container._max_packed_size = 10**6
+        try:
+            container.check_container_size()
+        except ContainerException:
+            self.fail("An unexpected error occurred while checking file size")
+
+    def test_check_container_size__too_big(self):
+        container = BaseContainer(self.fn)
+        container._max_packed_size = 1
+        with self.assertRaises(ContainerException) as context:
+            container.check_container_size()
+            self.assertTrue("container file is too large " in context.exception.value)
+
+    def test_check_container_size__too_small(self):
+        container = BaseContainer(self.fn)
+        container._min_packed_size = 10**6
+        with self.assertRaises(ContainerException) as context:
+            container.check_container_size()
+            self.assertTrue("container file is too small " in context.exception.value)
+
+    # Test check unpacked size
+    def _mock_calculate_unpacked_size(self):
+        return 10**3
+
+    def test_check_unpacked_size__happy(self):
+        container = BaseContainer(self.fn)
+        container.calculate_unpacked_size = self._mock_calculate_unpacked_size
+        container._min_unpacked_size = 0
+        container._max_unpacked_size = 10**6
+        try:
+            container.check_unpacked_size()
+        except ContainerException:
+            self.fail("An unexpected error occurred while checking file size")
+
+    def test_check_unpacked_size__too_big(self):
+        container = BaseContainer(self.fn)
+        container.calculate_unpacked_size = self._mock_calculate_unpacked_size
+        container._max_unpacked_size = 1
+        with self.assertRaises(ContainerException) as context:
+            container.check_unpacked_size()
+            self.assertTrue("container file is too large " in context.exception.value)
+
+    def test_check_unpacked_size__too_small(self):
+        container = BaseContainer(self.fn)
+        container.calculate_unpacked_size = self._mock_calculate_unpacked_size
+        container._min_unpacked_size = 10**6
+        with self.assertRaises(ContainerException) as context:
+            container.check_unpacked_size()
+            self.assertTrue("container file is too small " in context.exception.value)
+
     # Test get file
     def test_get_file__happy(self):
         try:
             container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
             snap_yaml = container.get_file("meta/snap.yaml")
             self.assertEqual(snap_yaml.decode(), "Test")
         except ContainerException:
@@ -55,6 +134,7 @@ class TestBaseContainer(unittest.TestCase):
     def test_get_file__missing_file(self):
         with self.assertRaises(ContainerException) as context:
             container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
             container.get_file("non/existent/file")
             self.assertTrue("Could not find" in context.exception.value)
 
@@ -62,6 +142,7 @@ class TestBaseContainer(unittest.TestCase):
     def test_get_files_list__happy(self):
         try:
             container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
             self.assertCountEqual(
                 container.get_files_list(),
                 self.expected_files,
@@ -72,6 +153,7 @@ class TestBaseContainer(unittest.TestCase):
     def test_get_files_list_abs__happy(self):
         try:
             container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
             self.assertCountEqual(
                 container.get_files_list(abs=True),
                 [
@@ -85,6 +167,7 @@ class TestBaseContainer(unittest.TestCase):
     def test_files__happy(self):
         try:
             container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
             self.assertCountEqual(
                 container.files,
                 self.expected_files,
@@ -96,6 +179,7 @@ class TestBaseContainer(unittest.TestCase):
     def test_get_compiled_binaries_list__happy(self):
         try:
             container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
             self.assertCountEqual(
                 container.get_compiled_binaries_list(), self.expected_binaries
             )
@@ -107,6 +191,7 @@ class TestBaseContainer(unittest.TestCase):
     def test_get_compiled_binaries_list_abs__happy(self):
         try:
             container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
             self.assertCountEqual(
                 container.get_compiled_binaries_list(abs=True),
                 [
@@ -122,6 +207,7 @@ class TestBaseContainer(unittest.TestCase):
     def test_bin_files__happy(self):
         try:
             container = BaseContainer(self.fn)
+            container._unpack_dir = self.unpack_dir
             self.assertCountEqual(container.bin_files, self.expected_binaries)
         except ContainerException:
             self.fail(
diff --git a/reviewtools/tests/containers/test_squashfs_container.py b/reviewtools/tests/containers/test_squashfs_container.py
new file mode 100644
index 0000000..7d19a77
--- /dev/null
+++ b/reviewtools/tests/containers/test_squashfs_container.py
@@ -0,0 +1,73 @@
+import os
+import unittest
+from tempfile import mkstemp, mkdtemp
+from reviewtools.containers.squashfs_container import (
+    SquashfsContainer,
+    ContainerException,
+)
+from reviewtools.tests.utils import make_snap2
+
+
+class TestSquashfsContainer(unittest.TestCase):
+    """Tests for reviewtools.containers.squashfs_container"""
+
+    @classmethod
+    def setUpClass(cls):
+        cls.tmp_dir = mkdtemp()
+        cls.fn = make_snap2(
+            output_dir=cls.tmp_dir,
+            yaml="Test",
+            extra_files=["/bin/ls:bin/ls"],
+        )
+
+    #  Test initialization
+    def test_check_initialization__happy(self):
+        try:
+            SquashfsContainer(self.fn)
+        except Exception:
+            self.fail(
+                "An unexpected error occurred during SquashfsContainer initialization with container file"
+            )
+
+    def test_check_initialization__invalid_format(self):
+        with self.assertRaises(ContainerException) as context:
+            fd, plain_file = mkstemp()
+            SquashfsContainer(plain_file)
+            self.assertEqual(
+                "unsupported package format (not squashfs)", context.exception.value
+            )
+            os.unlink(plain_file)
+
+    # Test check format
+    def test_calculate_unpacked_size__happy(self):
+        try:
+            # unpacked size calculated is a bit smaller than actual size as it does not consider that
+            # folders require at least one complete block
+            container = SquashfsContainer(self.fn)
+            size = container.calculate_unpacked_size()
+            self.assertTrue(isinstance(size, int))
+        except Exception:
+            self.fail("An unexpected error occurred during unpacked size calculation")
+
+    # Test unpack
+    def test_unpack__happy(self):
+        try:
+            container = SquashfsContainer(self.fn)
+            container.unpack()
+        except Exception:
+            self.fail("An unexpected error occurred during unpack")
+
+    def test_unpack__force(self):
+        try:
+            container = SquashfsContainer(self.fn)
+            container.unpack()
+            container.unpack(force=True)
+        except Exception:
+            self.fail("An unexpected error occurred during unpack")
+
+    def test_unpack__no_force(self):
+        with self.assertRaises(ContainerException) as context:
+            container = SquashfsContainer(self.fn)
+            container.unpack()
+            container.unpack()
+            self.assertTrue(" exists. Aborting." in context.exception.value)