XPInstall ignores user's umask when installing files

RESOLVED FIXED

Status

Core Graveyard
Installer: XPInstall Engine
RESOLVED FIXED
13 years ago
2 years ago

People

(Reporter: Andrew Schultz, Assigned: dbaron)

Tracking

({fixed1.7})

Trunk
All
Linux
fixed1.7

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
When files are installed via XPInstall, the user's umask is ignored.  The
XPInstall engine should respect the user's umask setting.  The problem code is here:

nsZipArchive::ExtractFile
http://lxr.mozilla.org/mozilla/source/modules/libjar/nsZipArchive.cpp#641
nsJAR::Extract
http://lxr.mozilla.org/mozilla/source/modules/libjar/nsJAR.cpp#251

both open the file with 0644 perms and then chmod the file to the appropriate
perms, but this sidesteps any umask.

Just to make things more complicated, the nsZipArchive is part of standalone
libjar, where PR_Open is defined as fopen in zipstub.h, so passing the file's
mode to PR_Open wouldn't work in that situation.

this bug contributed to problems in bug 231083.
(Reporter)

Comment 1

13 years ago
also note that (as argued against in bug 111124), directories are created with
appropriately umasked permissions here:
http://lxr.mozilla.org/seamonkey/source/xpinstall/src/nsInstallFile.cpp#213
(Reporter)

Comment 2

13 years ago
Created attachment 142404 [details] [diff] [review]
dbaron's patch minus a chmod

this is dbaron's patch from bug 231083 except that this patch also removes the
chmod from nsJAR::Extract.  This would fix things except for the standalone
libjar/PR_Open problem.
(Assignee)

Comment 3

13 years ago
I'd actually wanted to add an nsIFile::Remove / PR_Delete before the PR_Open /
GetANSIFileDesc calls so that it would be creating a new file rather than
reusing an old one that might have different permissions.  Otherwise a XPI
couldn't fix permissions that were wrong in an old buggy one, I think.
(Reporter)

Comment 4

13 years ago
Created attachment 142413 [details] [diff] [review]
add PR_TRUNCATE and workaround for standalone

this adds PR_TRUNCATE to the flags so that permissions are set for existing
files.	

this patch also grabs the umask and uses it to chmod the extracted file when
using standalone libjar.
Attachment #142404 - Attachment is obsolete: true
(Reporter)

Updated

13 years ago
Attachment #142413 - Flags: review?(bsmedberg)
Comment on attachment 142413 [details] [diff] [review]
add PR_TRUNCATE and workaround for standalone

I don't know enough about *nix file systems and umask sto review this.
Leaf/dbaron/dveditz?
Attachment #142413 - Flags: review?(bsmedberg) → review?(leaf)
(Assignee)

Comment 6

13 years ago
How does this work around the problem for standalone?  Anyway, I attached a
patch that does this the cheap way to bug 231803.
(Assignee)

Comment 7

13 years ago
er, bug 231083.
(Assignee)

Comment 8

13 years ago
Oh, I do see the workaround for standalone now, but I'd prefer that in a PR_Open.

Did you test that PR_TRUNCATE causes the permissions to be used if the file
exists?  Based on the documentation of open(2), I'd guess that it wouldn't do so.
(Assignee)

Comment 9

13 years ago
(In reply to comment #8)
> Oh, I do see the workaround for standalone now, but I'd prefer that in a PR_Open.

Actually, never mind, that's hard.
(Assignee)

Comment 10

13 years ago
But I'll attach a slightly improved version of this patch to bug 231083 tomorrow.
(Reporter)

Comment 11

13 years ago
(In reply to comment #8)
> Did you test that PR_TRUNCATE causes the permissions to be used if the file
> exists?  Based on the documentation of open(2), I'd guess that it wouldn't do so.

The documentation itself is ambiguous, but it worked ok when I actually tested
it (I can't retest right now).  It might not be portable though.
(Assignee)

Comment 12

13 years ago
It doesn't seem portable to Fedora Core 2.  I just did the following test.  I
added this to a test program:

PR_Open("/tmp/mozpermtest", PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE, 0700);

ran the test program, and it created /tmp/mozpermtest with 0700 (my umask is 002).

Then I changed the line to this:

PR_Open("/tmp/mozpermtest", PR_WRONLY | PR_CREATE_FILE | PR_TRUNCATE, 0666);

and ran the test program again, and the file was still 0700.
(Assignee)

Updated

13 years ago
Assignee: xpi-engine → dbaron
(Assignee)

Comment 13

13 years ago
Fixed by checkin of patch on bug 231083 to trunk, 2004-08-16 17:12 -0700.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

13 years ago
Fixed by checkin of bug 231083 to MOZILLA_1_7_BRANCH, 2004-08-17 13:24 -0700.
Keywords: fixed1.7
Comment on attachment 142413 [details] [diff] [review]
add PR_TRUNCATE and workaround for standalone

so this review request is obsolete, right?
Attachment #142413 - Flags: review?(leaf)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.