Merge ~jslarraz/review-tools:container-size-checks into review-tools:master

[Jorge Sancho Larraz]

Add size check functions to base_container

[Alex Murray (alexmurray)]

Can you provide some more rationale for this change?

review: Needs Information

[Jorge Sancho Larraz (jslarraz)]

So in a general case, we may want to set up the valid size limits for the container file and for the unpacked content. It is already true for snaps.

For snaps we are checking the size via a set of functions in common.py (see inline comments), however this functionality is intrinsic to the container and only makes sense in the context of it. Thus, I think it should be part of the base_container class.
The specific values for {min,max}_{,un}packed_size limits will be defined in each subclass (such as SnapContainer, ComponentContainer), but the functionality will be the same

There is some interaction among check_container_size and unpack, so I'm leaving the original functionality in comomon.py for now and it will removed in a follow up MR where unpack funtionality will be moved to the corresponding container class

[Alex Murray (alexmurray)]

Thanks for the pointers but perhaps I am missing something, but I don't see any users of these new methods - as such, perhaps it would help to also include the changes required to port the existing size checking / limits code to use this new interface?

[Jorge Sancho Larraz (jslarraz)]

Updated to move unpack to SquashfsContainer, which uses size checks

[Alex Murray (alexmurray)]

In general this looks really good - just a few minor things to address if you can thanks @jslarraz

review: Needs Fixing

[Jorge Sancho Larraz (jslarraz)]

Comments addressed in https://git.launchpad.net/~jslarraz/review-tools/commit/?id=de2de8651408f287c5c1b0a27787e2e3511b6057

Fix for container size not being checked in SnapContainer in https://git.launchpad.net/~jslarraz/review-tools/commit/?id=0cd400e35ad2b1637c02fd5463e5ca9bfaf559d9

> "I assume all containers would want to check their format - as such, should this be done in the BaseContainer constructor instead?"

I fully agree with you but it will require some non-obvious changes in test_base_container, thus I would prefer to do it in a followup MR

[Alex Murray (alexmurray)]

> I fully agree with you but it will require some non-obvious changes in test_base_container, thus I would prefer to do it in a followup MR

Sounds good - thanks @jslarraz - LGTM!

review: Approve

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..15d4d7e 100644
--- a/reviewtools/containers/base_container.py
+++ b/reviewtools/containers/base_container.py
@@ -8,11 +8,19 @@ 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"""
 
@@ -42,6 +50,30 @@ class BaseContainer:
         "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 +91,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 +102,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 +138,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 +262,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)
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)