classification
Title: zipfile.writestr doesn't set external attributes, so files are extracted mode 000 on Unix
Type: behavior Stage:
Components: Extension Modules Versions: Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: cbrannon, dkbg, mark, pitrou, swarren
Priority: high Keywords: patch

Created on 2008-07-17 19:01 by swarren, last changed 2008-07-25 19:44 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
writestr_usable_permissions.diff cbrannon, 2008-07-18 06:15
Messages (7)
msg69898 - (view) Author: Stephen Warren (swarren) Date: 2008-07-17 19:01
Run the following Python script, on Unix/Linux:

==========
import zipfile

z = zipfile.ZipFile('zipbad.zip', 'w')
z.writestr('filebad.txt', 'Some content')
z.close()

z = zipfile.ZipFile('zipgood.zip', 'w')
zi = zipfile.ZipInfo('filegood.txt')
zi.external_attr = 0660 << 16L
z.writestr(zi, 'Some content')
z.close()
==========

Like this:

python testzip.py  && unzip zipbad.zip && unzip zipgood.zip && ls -l
file*.txt

You'll see:

----------  1 swarren swarren   12 2008-07-17 12:54 filebad.txt
-rw-rw----  1 swarren swarren   12 1980-01-01 00:00 filegood.txt

Note that filebad.txt is extracted with mode 000.

The WAR (used for filegood.txt) is to pass writestr a ZipInfo class with
external_attr pre-initialized. However, writestr should perform this
assignment itself, to be consistent with write. I haven't checked, but
there's probably a bunch of other stuff in write that writestr should do
too.
msg69899 - (view) Author: Stephen Warren (swarren) Date: 2008-07-17 19:02
Oops. Forgot to set "type" field.
msg69922 - (view) Author: Christopher Brannon (cbrannon) Date: 2008-07-17 22:46
What value should the new archive entry's external_attr attribute have?
ZipFile.write sets it to the permissions of the file being archived, but
writestr is archiving a string, not a file.  Setting it to 0600 &lt;&lt; 16
seems reasonable.

Stephen's script exposed a second bug in writestr.  When passed a name
rather than a ZipInfo instance, the new archive member receives a timestamp
of 01/01/1980.  However, the docs say that the timestamp should correspond to
the current date and time.
ZipFile.writestr begins with the following code:

    def writestr(self, zinfo_or_arcname, bytes):
        """Write a file into the archive.  The contents is the string
        'bytes'.  'zinfo_or_arcname' is either a ZipInfo instance or
        the name of the file in the archive."""
        if not isinstance(zinfo_or_arcname, ZipInfo):
            zinfo = ZipInfo(filename=zinfo_or_arcname,
                            date_time=time.localtime(time.time())[:6])
            zinfo.compress_type = self.compression

The "date_time=" line should read:

                            zinfo.date_time=time.localtime(time.time())[:6])
msg69932 - (view) Author: Stephen Warren (swarren) Date: 2008-07-18 03:57
I'd probably argue for at least 0660<<16, if not 0666<<16, since group
permissions are pretty typically set, but even 0666<<16 would be OK,
since the umask on extraction would take away any permissions the
extracting user didn't want.

But, as long as the chosen mask includes at least 0600, I'd consider the
issue fixed.
msg69937 - (view) Author: Christopher Brannon (cbrannon) Date: 2008-07-18 06:15
Here is a patch containing code and a unit test.  I set external_attr
to 0600, for the following reason.
When I extract with Infozip, my umask is ignored when setting permissions of
extracted entries.  They have the permissions assigned to them when archived.
tar does respect umask, but it's not pertinent.
The following shell script demonstrates Infozip's behavior:

=====begin=====
#!/bin/sh
mkdir ziptest_dir
echo hello > ziptest_dir/foo.txt
chmod 666 ziptest_dir/foo.txt
zip -r ziptest_file.zip ziptest_dir/
rm -rf ziptest_dir
umask 077
unzip ziptest_file.zip
=====end=====

Setting permissions to 0600 seems like the safest course.

I'm not sure if this patch should be accompanied by some documentation,
since the zipfile docs don't say much about external_attr or permissions.

PS.  My earlier comments about timestamps were incorrect and spurious!
msg70245 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-07-25 09:57
Agree with using 0600 as default permissions.
msg70271 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-07-25 19:44
Committed in r65235. Thanks!
History
Date User Action Args
2010-09-14 11:18:39amaury.forgeotdarclinkissue9835 superseder
2008-07-25 19:44:31pitrousetstatus: open -> closed
resolution: fixed
messages: + msg70271
2008-07-25 10:26:31marksetnosy: + mark
2008-07-25 09:57:31pitrousetpriority: high
nosy: + pitrou
messages: + msg70245
2008-07-24 16:41:04dkbgsetnosy: + dkbg
2008-07-18 06:15:58cbrannonsetfiles: + writestr_usable_permissions.diff
keywords: + patch
messages: + msg69937
2008-07-18 03:57:35swarrensetmessages: + msg69932
2008-07-17 22:47:00cbrannonsetnosy: + cbrannon
messages: + msg69922
versions: + Python 2.6
2008-07-17 19:02:02swarrensettype: behavior
messages: + msg69899
2008-07-17 19:01:33swarrencreate