Fix the permission setting in common.ZipWriteStr()

When passing a ZipInfo instance to common.ZipWriteStr(), the
external_attr attribute should not be overwritten unless specified.
We didn't have the issue previously because we were calling
ZipFile.writestr() directly until [1] merged.

[1] commit 2ed665a033c587b276b1615516e5354e2ace47cd.

Bug: http://b/21309935
Change-Id: I8c0190362c60d7d78965ecfe5e484f8398ddc5f2
(cherry picked from commit 97734654099431bd6c5bd2eeb5d34af0e2dc03e7)

2 files changed, 50 insertions, 7 deletions

diff --git a/tools/releasetools/common.py b/tools/releasetools/common.py
index 8ab98e7..4f85491 100644
--- a/tools/releasetools/common.py
+++ b/tools/releasetools/common.py
@@ -862,7 +862,7 @@ def ZipWrite(zip_file, filename, arcname=None, perms=0o644,
     zipfile.ZIP64_LIMIT = saved_zip64_limit
 
 
-def ZipWriteStr(zip_file, zinfo_or_arcname, data, perms=0o644,
+def ZipWriteStr(zip_file, zinfo_or_arcname, data, perms=None,
                 compress_type=None):
   """Wrap zipfile.writestr() function to work around the zip64 limit.
 
@@ -881,6 +881,8 @@ def ZipWriteStr(zip_file, zinfo_or_arcname, data, perms=0o644,
   if not isinstance(zinfo_or_arcname, zipfile.ZipInfo):
     zinfo = zipfile.ZipInfo(filename=zinfo_or_arcname)
     zinfo.compress_type = zip_file.compression
+    if perms is None:
+      perms = 0o644
   else:
     zinfo = zinfo_or_arcname
 
@@ -888,8 +890,11 @@ def ZipWriteStr(zip_file, zinfo_or_arcname, data, perms=0o644,
   if compress_type is not None:
     zinfo.compress_type = compress_type
 
+  # If perms is given, it has a priority.
+  if perms is not None:
+    zinfo.external_attr = perms << 16
+
   # Use a fixed timestamp so the output is repeatable.
-  zinfo.external_attr = perms << 16
   zinfo.date_time = (2009, 1, 1, 0, 0, 0)
 
   zip_file.writestr(zinfo, data)
diff --git a/tools/releasetools/test_common.py b/tools/releasetools/test_common.py
index f28934d..a861346 100644
--- a/tools/releasetools/test_common.py
+++ b/tools/releasetools/test_common.py
@@ -121,15 +121,18 @@ class CommonZipTest(unittest.TestCase):
       time.sleep(5)  # Make sure the atime/mtime will change measurably.
 
       if not isinstance(zinfo_or_arcname, zipfile.ZipInfo):
-        zinfo = zipfile.ZipInfo(filename=zinfo_or_arcname)
+        arcname = zinfo_or_arcname
+        expected_mode = extra_args.get("perms", 0o644)
       else:
-        zinfo = zinfo_or_arcname
-      arcname = zinfo.filename
+        arcname = zinfo_or_arcname.filename
+        expected_mode = extra_args.get("perms",
+                                       zinfo_or_arcname.external_attr >> 16)
 
-      common.ZipWriteStr(zip_file, zinfo, contents, **extra_args)
+      common.ZipWriteStr(zip_file, zinfo_or_arcname, contents, **extra_args)
       common.ZipClose(zip_file)
 
       self._verify(zip_file, zip_file_name, arcname, contents,
+                   expected_mode=expected_mode,
                    expected_compress_type=expected_compress_type)
     finally:
       os.remove(zip_file_name)
@@ -228,9 +231,10 @@ class CommonZipTest(unittest.TestCase):
     random_string = os.urandom(1024)
     # Passing arcname
     self._test_ZipWriteStr("foo", random_string, {
+        "perms": 0o700,
         "compress_type": zipfile.ZIP_DEFLATED,
     })
-    self._test_ZipWriteStr("foo", random_string, {
+    self._test_ZipWriteStr("bar", random_string, {
         "compress_type": zipfile.ZIP_STORED,
     })
 
@@ -240,6 +244,7 @@ class CommonZipTest(unittest.TestCase):
         "compress_type": zipfile.ZIP_DEFLATED,
     })
     self._test_ZipWriteStr(zinfo, random_string, {
+        "perms": 0o600,
         "compress_type": zipfile.ZIP_STORED,
     })
 
@@ -257,3 +262,36 @@ class CommonZipTest(unittest.TestCase):
     self._test_reset_ZIP64_LIMIT(self._test_ZipWriteStr, "foo", "")
     zinfo = zipfile.ZipInfo(filename="foo")
     self._test_reset_ZIP64_LIMIT(self._test_ZipWriteStr, zinfo, "")
+
+  def test_bug21309935(self):
+    zip_file = tempfile.NamedTemporaryFile(delete=False)
+    zip_file_name = zip_file.name
+    zip_file.close()
+
+    try:
+      random_string = os.urandom(1024)
+      zip_file = zipfile.ZipFile(zip_file_name, "w")
+      # Default perms should be 0o644 when passing the filename.
+      common.ZipWriteStr(zip_file, "foo", random_string)
+      # Honor the specified perms.
+      common.ZipWriteStr(zip_file, "bar", random_string, perms=0o755)
+      # The perms in zinfo should be untouched.
+      zinfo = zipfile.ZipInfo(filename="baz")
+      zinfo.external_attr = 0o740 << 16
+      common.ZipWriteStr(zip_file, zinfo, random_string)
+      # Explicitly specified perms has the priority.
+      zinfo = zipfile.ZipInfo(filename="qux")
+      zinfo.external_attr = 0o700 << 16
+      common.ZipWriteStr(zip_file, zinfo, random_string, perms=0o400)
+      common.ZipClose(zip_file)
+
+      self._verify(zip_file, zip_file_name, "foo", random_string,
+                   expected_mode=0o644)
+      self._verify(zip_file, zip_file_name, "bar", random_string,
+                   expected_mode=0o755)
+      self._verify(zip_file, zip_file_name, "baz", random_string,
+                   expected_mode=0o740)
+      self._verify(zip_file, zip_file_name, "qux", random_string,
+                   expected_mode=0o400)
+    finally:
+      os.remove(zip_file_name)