zipfile.writestr doesn't set external attributes, so files are extracted mode 000 on Unix

[swarren]

BPO	3394
Nosy	@pitrou, @mark-summerfield
Files	writestr_usable_permissions.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2008-07-25.19:44:31.760>
created_at = <Date 2008-07-17.19:01:32.915>
labels = ['extension-modules', 'type-bug']
title = "zipfile.writestr doesn't set external attributes, so files are extracted mode 000 on Unix"
updated_at = <Date 2008-07-25.19:44:31.759>
user = 'https://bugs.python.org/swarren'

bugs.python.org fields:

activity = <Date 2008-07-25.19:44:31.759>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2008-07-25.19:44:31.760>
closer = 'pitrou'
components = ['Extension Modules']
creation = <Date 2008-07-17.19:01:32.915>
creator = 'swarren'
dependencies = []
files = ['10933']
hgrepos = []
issue_num = 3394
keywords = ['patch']
message_count = 7.0
messages = ['69898', '69899', '69922', '69932', '69937', '70245', '70271']
nosy_count = 5.0
nosy_names = ['pitrou', 'mark', 'swarren', 'cbrannon', 'dkbg']
pr_nums = []
priority = 'high'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue3394'
versions = ['Python 2.6', 'Python 2.5']

[swarren]

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.

[swarren]

Oops. Forgot to set "type" field.

[cbrannon]

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 << 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])

[swarren]

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.

[cbrannon]

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!

[pitrou]

Agree with using 0600 as default permissions.

[pitrou]

Committed in r65235. Thanks!