446464 - File permissions are not preserved when zipping with zipwriter

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008071803 Thunderbird/3.0a2pre ID:2008071803

After the check-in of the patch on bug 372786 I noticed following behavior:

Using the given attachment 307796 [details] the keynote folder content gets zipped and is
received correctly as zip file. After unpacking the content everything looks
identical. As I see there is one remaining issue when having a more closely
look at the files:

Original keynote file:
drwxr-xr-x@ 3 henrik  staff    102  6 Mär 21:52 Contents
-rw-r--r--@ 1 henrik  staff  83487  6 Mär 21:52 index.apxl.gz
drwxr-xr-x@ 3 henrik  staff    102  6 Mär 21:52 thumbs

Received keynote file:
drwxr-xr-x@ 3 henrik  staff    102  6 Mär 20:52 Contents
-rwxr-xr-x@ 1 henrik  staff  83487  6 Mär 20:52 index.apxl.gz
drwxr-xr-x@ 3 henrik  staff    102  6 Mär 20:52 thumbs

Each file is packed with the executable flag so we have modified permissions
afterwards.

Comment 1

[Håkan Waara]

Mossop, any idea if the zipwriter cares about the executable bit on directories and files? See comment 0 in this bug and also bug 372786 comment 50.

I haven't started investigating this yet, but we're simply using addEntryFile to add the files (http://mxr.mozilla.org/mozilla/source/mailnews/compose/src/nsMsgAttachmentHandler.cpp#120), so I don't see why permissions would change, unless some other mailnews code is changing them. 

Comment 2

[Dave Townsend [:mossop]]

(In reply to comment #1)
> Mossop, any idea if the zipwriter cares about the executable bit on directories
> and files? See comment 0 in this bug and also bug 372786 comment 50.

It does not. The basic zip specification does not understand executable attributes or anything either.

Maybe look at what zip tool is extracting as well?

Comment 3

[Håkan Waara]

So it seems to be a limitation of our zipwriter that it does not preserve file permissions. The built-in OS X zipper does that, and that's why it's working differently.

Moving to the right component.

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #2)
> (In reply to comment #1)
> > Mossop, any idea if the zipwriter cares about the executable bit on directories
> > and files? See comment 0 in this bug and also bug 372786 comment 50.
> 
> It does not. The basic zip specification does not understand executable
> attributes or anything either.

Apparently I'm wrong on this. The ZIP spec allows for attributes to be stored in an OS dependant manner. zipwriter currently just stores attributes in DOS form, while zipreader ignores the DOS attributes and always interprets from a unix point of view.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This implements a basic level of file permission storage in the zip file. It does it in a manner that would make it easy to include permissions in the interface at a future point but since we are past b1 now I don't think it is worth revving the interface till after 1.9.1.

The permissions are held in the external file attributes field of the zip header. Low word is DOS attributes, high word is the Unix file mode. I.ve marked the zip file as being generated on a Unix system to be more sure that other zip tools will then recognise the permissions. This doesn't affect whether windows and other systems can extract the zip files.

The patch basically adds passing permissions around, using default sets except for AddEntryFile where we get the real permissions from the file.

The unit test adds files to a zip file with as many different permissions as possible. At a minimum we add the user read bit because nsIZipReader always adds this bit on extraction. To get around the umask and platfor differences we create a file with the intended permissions then check what permissions it really gets. OSX and Unix cope well, coverage is high. Windows coverage is essentially non-existant unfortunately, permissions are just always 666 there.

Also just rolled in a simplification of another unit test that was creating and not deleting temporary files when it didn't need to.

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 341296 [details] [diff] [review]
patch rev 1

*mutters about bugzilla*

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 341296 [details] [diff] [review]
patch rev 1


+    header->Init(aZipEntry, aModTime, ZIP_ATTRS(aPermissions, ZIP_ATTRS_FILE), mCDSOffset);

please limit your lines to 80 characters

(also on a few other lines)


+    PRUint32 permissions = ZIP_ATTRS(aPermissions, ZIP_ATTRS_DIRECTORY);

since these are not the original permissions I think a better name would be zipAttributes or something like that.

+    PRUint32 permissions = ZIP_ATTRS(aItem->mPermissions, ZIP_ATTRS_FILE);

here too

Comment 8

[Dave Townsend [:mossop]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/e652dc334cfa