702726 - unpacked XPIs don't retain +x permissions on data/ files during 'cfx run'

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Brian Warner [:warner :bwarner]]

Dan Walkowski reported that 'cfx run' with current SDK trunk (revid 6423793) appears to clobber file permissions on bundled data files. In particular, he's got an addon that includes a "xulrunner" executable in the source package's data/ directory, with the executable bits set. When he runs 'cfx run', and looks in the temporary profile, the unpacked XPI files have lost the +x bit.

This is a change since 1.3 because the new code now always creates an XPI, even for 'cfx run', whereas the older code allowed 'cfx run' to read addon code (and data) directly from the source tree, and no XPI was built.

I took a quick look at the zipfile created by 'cfx xpi', and the data/ files therein have the right permissions. Unpacking the XPI with 'unzip' also retains the permissions.

It's possible that this is a FF bug (the internal XPI unpacker might not honor file-mode bits on zipfile contents), which which case it may be a hassle to fix. Also, the use-case of including executables in addons needs to be thought through, since that will come into conflict with our plans to not unpack XPIs (for speed and other benefits).

Comment 1

[Brian Warner [:warner :bwarner]]

Mossop pointed me at http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#1057 (which shows that the addon-installer only adds file-permission bits, never removes them), and the similar bug 541420 (fixed forever ago).

Neither looks like the unpacked XPI should have its +x bits stripped, but that's what I'm seeing in a local test against both FF-7.0.1 and FF-9.0 (the current beta).

Comment 2

[Brian Warner [:warner :bwarner]]

Ah, thanks to Mossop's suggestion, I looked at the way that our copy of
mozrunner is installing the XPIs into the temporary profile. It turns out
that it's manually extracting each file in turn, which doesn't pay attention
to the permission bits:

https://github.com/mozilla/addon-sdk/blob/6423793c6adff424c9129f502de6d90e402980ee/python-lib/mozrunner/__init__.py#L192

    tmpdir = tempfile.mkdtemp(suffix = "." + os.path.split(addon)[-1])
    compressed_file = zipfile.ZipFile(addon, "r")
    for name in compressed_file.namelist():
        if name.endswith('/'):
            makedirs(os.path.join(tmpdir, name))
        else:
            if not os.path.isdir(os.path.dirname(os.path.join(tmpdir, name))):
                makedirs(os.path.dirname(os.path.join(tmpdir, name)))
            data = compressed_file.read(name)
            f = open(os.path.join(tmpdir, name), 'wb')
            f.write(data) ; f.close()


ZipFile offers .extract() and .extractAll() methods which will probably set
the permission bits for you, although they're marked in the docs as being
unsafe with respect to archive files with names like "../surprise" and
"/etc/password".

So the fix is probably going to be one of:
 1: use extractAll and accept the unsafety (since we're only unpacking XPIs
    that were generated a moment earlier elsewhere in jetpack)
 2: scan the filenames first, throw an error if we see leading "/" or any
    "..", then use extractAll
 3: continue to copy out files one at a time, but read the zipfile attributes
    and attempt to set them on the generated file

Comment 3

[Brian Warner [:warner :bwarner]]

also, there is probably a newer version of mozrunner.. we should look at that before making changes to our own copy, and try to keep in sync:

 https://github.com/mozilla/mozbase/

Comment 4

[Brian Warner [:warner :bwarner]]

Sigh, the 'zipfile' module in python's stdlib (at least up through py2.7) doesn't extract file-permission bits in ZipFile.extract() or ZipFile.extractAll(). It records them in .write(), but ignores them during read. So the first two options are out.

Comment 5

[Brian Warner [:warner :bwarner]]

Attachment: fix file modes during 'cfx run' unpack

here's a quick patch.. not yet convinced this is the direction we want to go, but it might unblock anyone suffering from this bug

Comment 6

[Dan Walkowski]

regardless, cfx run and cfx xpi should generate the same set of files.

Comment 7

[Brian Warner [:warner :bwarner]]

yeah, they do generate the same thing (at least starting in 1.4). The problem is entirely in the way that jetpack's "cfx run" installs the addon into the temporary profile.

I took a quick look at upstream mozrunner (in the 'mozbase' repo). It's drastically rearranged from our current version (about 300kB in 36 files, versus our current 60kB in 5 files), and still suffers from the ignoring-permission-bits problem. It'll take some non-trivial work to switch to it, and then we'd still need to apply this two-line patch to the new version.

So I'm ok with applying this change to our local copy, submitting it upstream, then once upstream has accepted it and made a new release, start the process of upgrading our copy to the new upstream.

Comment 8

[Brian Warner [:warner :bwarner]]

Comment on attachment 574695 [details] [diff] [review]
fix file modes during 'cfx run' unpack

ok, looking for r? on this now

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 574695 [details] [diff] [review]
fix file modes during 'cfx run' unpack

This seems reasonable and works in my tests on Windows, where os.chmod is mostly ignored <http://docs.python.org/library/os.html#os.chmod>. r=myk

I did see three failures while testing the patch on Windows via `cfx testex`, all of which occurred in distinct runs of the reading-data tests; but I am unable to reproduce them:

    Testing reading-data...
    Using binary at 'C:\Program Files (x86)\Mozilla Firefox\firefox.exe'.
    Using profile at 'c:\users\myk\appdata\local\temp\tmpvebaxx.mozrunner'.
    Running tests on Firefox 8.0/Gecko 8.0 ({ec8030f7-c20a-464f-9b0e-13a3a9e97384}) under winnt/x86-msvc.
    ...info: My ID is [e-mail]
    
    3 of 3 tests passed.
    Traceback (most recent call last):
      File "c:/Users/myk/Dropbox/addon-sdk/bin/cfx", line 29, in <module>
        cuddlefish.run()
      File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\__init__.py", line 476, in run
        test_all_examples(env_root, defaults=options.__dict__)
      File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\__init__.py", line 364, in test_all_examples
        env_root=env_root)
      File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\__init__.py", line 771, in run
        mobile_app_name=options.mobile_app_name)
      File "c:\Users\myk\Dropbox\addon-sdk\python-lib\cuddlefish\runner.py", line 606, in run_app
        OUTPUT_TIMEOUT)
    Exception: Test output exceeded timeout (60s).
    
    
    Testing reading-data...
    Error Cleaning up: [Error 32] The process cannot access the file because it is being used by another process: 'c:\\users\\myk\\appdata\\local\\temp\\tmpu36ter.mozrunner\\chromeappsstore.sqlite'
    Error Cleaning up: [Error 32] The process cannot access the file because it is being used by another process: 'c:\\users\\myk\\appdata\\local\\temp\\tmpu36ter.mozrunner\\content-prefs.sqlite'
    Error Cleaning up: [Error 2] The system cannot find the file specified: 'c:\\users\\myk\\appdata\\local\\temp\\tmpu36ter.mozrunner\\parent.lock'
    Error Cleaning up: [Error 145] The directory is not empty: 'c:\\users\\myk\\appdata\\local\\temp\\tmpu36ter.mozrunner'
    Using binary at 'c:/Program Files/Nightly/firefox.exe'.
    
    
    Testing reading-data...
    Error Cleaning up: [Error 145] The directory is not empty: 'c:\\users\\myk\\appdata\\local\\temp\\tmpe0eqz5.mozrunner\\extensions\\jid1-R4rSVNkBANnvGQ@jetpack\\resources'
    Error Cleaning up: [Error 145] The directory is not empty: 'c:\\users\\myk\\appdata\\local\\temp\\tmpe0eqz5.mozrunner\\extensions\\jid1-R4rSVNkBANnvGQ@jetpack'
    Error Cleaning up: [Error 145] The directory is not empty: 'c:\\users\\myk\\appdata\\local\\temp\\tmpe0eqz5.mozrunner\\extensions'
    Error Cleaning up: [Error 145] The directory is not empty: 'c:\\users\\myk\\appdata\\local\\temp\\tmpe0eqz5.mozrunner'
    Using binary at 'C:\Program Files (x86)\Mozilla Firefox\firefox.exe'.

Comment 10

[[github robot]]

Commit pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/23bdd0edfe022c9347f84557b0a333100581e40e
Bug 702726: preserve XPI mode bits during 'cfx run'. r=myk

Without this, executables in data/ directories lose their chmod +x during
'cfx run', even though the mode bits in the zipfile are correct. Mozrunner
was not paying attention to the mode bits when unpacking the XPI.