Closed Bug 1233434 Opened 8 years ago Closed 8 years ago

Linux: temporary download directory is not created specifically for each user

Categories

(Core :: Networking: File, defect)

43 Branch
Unspecified
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox42 --- unaffected
firefox43 + fixed
firefox44 + verified
firefox45 + verified
firefox46 + verified
firefox-esr38 --- unaffected
b2g-v2.5 --- fixed

People

(Reporter: mschiffer+mozilla, Assigned: aidin)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151216131022

Steps to reproduce:

We are using Firefox in a multi-user Ubuntu environment (but other distros like Arch are affected as well).

To reproduce:
1. Have one user log in
2. Start Firefox
3. Download a file
4. Logout
5. Log in as another user
6. Try to download a file again


Actual results:

The second download failed instantaneously.

Firefox had created a directory called /tmp/mozilla_mozillaUser0 for the first user. Then it tried to use the same temporary directory as the second user, which obviously failed.


Expected results:

The download should have worked.

Looking at the code, there are two different bugs in GetDownloadDirectory in uriloader/exthandler/nsExternalHelperAppService.cpp:

1. The $USER environment variable is not respected; the code replaces its value with "mozillaUser", if only $USER, but not $USERNAME or $LOGNAME are set. Possible fix:

--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -416,12 +416,12 @@ static nsresult GetDownloadDirectory(nsIFile **_directory,
     const char* userName = PR_GetEnv("USERNAME");
     if (!userName || !*userName) {
       userName = PR_GetEnv("USER");
-      if (!userName || !*userName) {
-        userName = PR_GetEnv("LOGNAME");
-      }
-      else {
-        userName = "mozillaUser";
-      }
+    }
+    if (!userName || !*userName) {
+      userName = PR_GetEnv("LOGNAME");
+    }
+    if (!userName || !*userName) {
+      userName = "mozillaUser";
     }
 
     nsAutoString userDir;

2. The permissions of the temporary directory are checked, but not the owner, so the fallback counter after the username isn't incremented even though the found directory is not usable. I don't know the code of Firefox well enough to propose a fix for this.
Component: Untriaged → Download Manager
Product: Firefox → Toolkit
Component: Download Manager → Networking: File
Product: Toolkit → Core
[Tracking Requested - why for this release]: Break downloading file in multi-user linux

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8b63529bd739e7d629426fbb337408d76f9c4988&tochange=20b84c6d55a0befeebd372be64497db318e7081f

Regressed by:
b046fd76907f	Aidin Gharibnavaz — Bug 377630 - Preventing filename disclosure, by putting downloaded files in a private directory. r=bz
Blocks: 377630
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Unspecified → Linux
Flags: needinfo?(aidin)
Hi.

I did report the same bug in 1233547, but i did also suggest a quick fix. It is enough to create TMP environment variable to point to existing directory (ie. /tmp/userx), and this bug does not show itself.

What it is odd, is that defining TMP variable does not create this directory (mozilla_mozillaUser0).

Regards,

H.
Sorry for that! I'll fix it soon.
Assignee: nobody → aidin
Flags: needinfo?(aidin)
Boris (:bz) is on vaccation, so I asked Patrik to kindly review the patch.

The try server build is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a7318522891

I will try to write more unit tests for it, tomorrow.
Attachment #8700078 - Flags: review?(mcmanus) → review?(daniel)
So it is still set on mcmanus in mozreview and I can't figure out how to move it to me, but it's a very small and straight-forward patch with a r+ from me, if I could set it.
Comment on attachment 8700078 [details]
MozReview Request: Bug 1233434 - Fixing download failure on a multi-user GNU/Linux machine. r?bagder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28565/diff/1-2/
Attachment #8700078 - Attachment description: MozReview Request: Bug 1233434 - Fixing download failure on a multi-user GNU/Linux machine. r?mcmanus → MozReview Request: Bug 1233434 - Fixing download failure on a multi-user GNU/Linux machine. r?bagder
Thanks (:
I updated the board by editing the commit message and pushing another commit.
Attachment #8700078 - Flags: review?(daniel) → review+
Comment on attachment 8700078 [details]
MozReview Request: Bug 1233434 - Fixing download failure on a multi-user GNU/Linux machine. r?bagder

https://reviewboard.mozilla.org/r/28565/#review25477

LGTM!
Thanks for the review (:
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/babff100e9f8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
I think the patch needs to be uplifted to aurora, beta and release so the fix goes to 43.0.3.
Flags: needinfo?(aidin)
Indeed. Please fill an uplift request to aurora and beta.
Comment on attachment 8700078 [details]
MozReview Request: Bug 1233434 - Fixing download failure on a multi-user GNU/Linux machine. r?bagder

Approval Request Comment
[Feature/regressing bug #]: Bug 377630, fails in multi-user environments.
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]: Manual test
[Risks and why]: low, changes have no side-effect on other things.
[String/UUID change made/needed]:none
Flags: needinfo?(aidin)
Attachment #8700078 - Flags: approval-mozilla-beta?
Attachment #8700078 - Flags: approval-mozilla-aurora?
Comment on attachment 8700078 [details]
MozReview Request: Bug 1233434 - Fixing download failure on a multi-user GNU/Linux machine. r?bagder

Seems like a key scenario, Beta44+, Aurora45+
Attachment #8700078 - Flags: approval-mozilla-beta?
Attachment #8700078 - Flags: approval-mozilla-beta+
Attachment #8700078 - Flags: approval-mozilla-aurora?
Attachment #8700078 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Reproduced the initial issue using Firefox 43 beta 9 on Ubuntu 14.04 64-bit, verified that using Firefox 44 beta 4, latest Developer Edition 45.0a2 and latest Nightly 46.0a1 the issue is fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
I didn't see this in time to get it into either 43.0.3. We have to rebuild 43.0.4 this afternoon so potentially could still take this.
Comment on attachment 8700078 [details]
MozReview Request: Bug 1233434 - Fixing download failure on a multi-user GNU/Linux machine. r?bagder

[Triage Comment]

Taking this for release as well as a ridealong for 43.0.4.
Attachment #8700078 - Flags: approval-mozilla-release+
This may not affect a huge percentage of installs relative to windows installs but seems likely to be something that affects university computer labs. The patch has been verified on other channels and has been on 44 for a couple of weeks without incident. 

Note also, this should have been an uplift request on the release channel as well; I missed it over the holidays in the backlog of tracking requests, which aren't usually a priority.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: