Unzipping contents before the enclosing directories (#388)

to prevent crash in cases where directories is missing READ/EXECUTE
permission

Added a test that
- creates a zip file with directories without READ/EXECUTE permissions
- `os.unzip` everything successfully
- re-adds READ and EXECUTE permissions to directories

see my and @lefou's comments
#387 (comment)
com-lihaoyi/mill#5048 (comment)

Tested with the android test. Here is the permission fix at the call
site (`AndroidApModule.scala`)

com-lihaoyi/mill@main...kiendang:mill:fix-android-classesjar-unzip#diff-49ebd9c68d1348785194e15a80d62f0845c26386e722d3f5d43429238bbf7616

@lihaoyi @vaslabs

Author: kiendang

os/src/ZipOps.scala

@@ -345,12 +345,11 @@ object unzip {
         )
       })
       .toList
-      .sortBy { case (_, path, _, isSymLink, _) =>
-        // Unzipping symbolic links last.
-        // Enclosing directories are unzipped before their contents.
-        // This makes sure directory permissions are applied correctly.
-        (isSymLink, path)
-      }
+      .sortBy { case (_, path, _, _, _) =>
+        // Unzipping children first before the enclosing directories
+        // This prevents crash for the case where directories lack READ/EXECUTE permission
+        path
+      }(Ordering[os.SubPath].reverse)
 
     try {
       for ((zipEntry, path, mode, isSymLink, zipInputStream) <- zipEntryInputStreams) {
@@ -360,10 +359,10 @@ object unzip {
         } else null
 
         if (zipEntry.isDirectory) {
-          os.makeDir.all(newFile)
-          if (perms != null) {
-            // make sure directories at least have OWNER_EXECUTE
-            os.perms.set(newFile, perms + PosixFilePermission.OWNER_EXECUTE)
+          os.makeDir.all(newFile, perms = perms)
+          if (perms != null && os.perms(newFile) != perms) {
+            // because of umask
+            os.perms.set(newFile, perms)
           }
         } else if (isSymLink) {
           val target = scala.io.Source.fromInputStream(zipInputStream).mkString

os/test/src/ZipOpTests.scala

@@ -538,24 +538,86 @@ object ZipOpTests extends TestSuite {
       assert(file2Content == "Content of file2")
     }
 
-    test("unzipDirectoryEnsureExecutablePermission") - prep { wd =>
+    test("unzipDirectoriesWithoutReadOrExecute") - prep { wd =>
       if (!scala.util.Properties.isWin) {
-        val zipFileName = "zipDirExecutable"
-        val source = wd / "folder1"
-        val dir = source / "dir"
+        def zip_(sources: Seq[(os.Path, os.PermSet)], root: os.Path, dest: os.Path): os.Path = {
+          import os.{shaded_org_apache_tools_zip => apache}
 
-        os.makeDir(dir)
-        val perms = os.perms(dir)
-        os.perms.set(dir, perms - PosixFilePermission.OWNER_EXECUTE)
+          val zipOut = new apache.ZipOutputStream(
+            java.nio.file.Files.newOutputStream(dest.toNIO)
+          )
 
-        val zipped = os.zip(
-          dest = wd / s"$zipFileName.zip",
-          sources = Seq(source)
+          try {
+            sources.foreach { case (p, perms) =>
+              val name = p.subRelativeTo(root).toString + (if (os.isDir(p)) "/" else "")
+
+              val fileType = apache.PermissionUtils.FileType.of(p.toNIO)
+              val mode = apache.PermissionUtils.modeFromPermissions(perms.toSet(), fileType)
+              val fis = if (os.isDir(p))
+                None
+              else Some(os.read.inputStream(p))
+
+              val zipEntry = new apache.ZipEntry(name)
+              zipEntry.setUnixMode(mode)
+
+              try {
+                zipOut.putNextEntry(zipEntry)
+                fis.foreach(os.Internals.transfer(_, zipOut, close = false))
+                zipOut.closeEntry()
+              } finally {
+                fis.foreach(_.close())
+              }
+            }
+            zipOut.finish()
+          } finally {
+            zipOut.close()
+          }
+
+          dest
+        }
+
+        def walk_(p: os.Path): geny.Generator[os.Path] = {
+          if (os.isDir(p))
+            os.list.stream(p) ++ os.list.stream(p).flatMap(walk_)
+          else geny.Generator()
+        }
+
+        import java.nio.file.attribute.PosixFilePermission._
+
+        val zipFileName = "zipDirNoReadExecute"
+        val source = wd / "dirNoReadExecute"
+        val dir = source / "dir"
+        val nested = dir / "nested"
+        val file = nested / "file.txt"
+
+        os.makeDir.all(nested)
+        os.write(file, "Contents of file.txt")
+
+        val readAndExecute = os.PermSet.fromSet(java.util.Set.of(
+          OWNER_READ,
+          OWNER_EXECUTE,
+          GROUP_READ,
+          GROUP_EXECUTE,
+          OTHERS_READ,
+          OTHERS_EXECUTE
+        ))
+
+        val filesToZip: Seq[(os.Path, os.PermSet)] =
+          Seq(dir, nested, file)
+            .map(p => (p, if (os.isDir(p)) os.perms(p) -- readAndExecute else os.perms(p)))
+
+        val zipped = zip_(filesToZip, source, wd / s"$zipFileName.zip")
+        val unzipped = os.unzip(
+          zipped,
+          dest = wd / zipFileName
         )
 
-        val unzipped = os.unzip(zipped, dest = wd / zipFileName)
-        assert(os.perms(unzipped / "dir").contains(PosixFilePermission.OWNER_EXECUTE))
-        assert(os.perms(unzipped / "dir") == perms)
+        walk_(unzipped).foreach { p =>
+          os.perms.set(p, os.perms(p) + OWNER_READ + OWNER_EXECUTE)
+        }
+
+        assert(os.walk(unzipped).map(_.subRelativeTo(unzipped)) ==
+          os.walk(source).map(_.subRelativeTo(source)))
       }
     }
   }