FF creates prefs-<n>.js files rapidly when prefs.js is write-protected with ACL

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: nikolay.khil, Assigned: njn)

Tracking

57 Branch
mozilla60
All
Windows
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36

Steps to reproduce:

1. Navigate to the FF user's profile directory (e.g. %appdata%\Mozilla\Firefox\Profiles\0gnuklgb.default)
2. Modify the ACL (Access Control List) for prefs.js, and deny write access to it
3. Start Firefox

Video: https://www.screencast.com/t/8lr9JVB3
Example of Deny ACE: https://www.screencast.com/t/vqVjXQY0Xh4l


Actual results:

Firefox starts creating prefs-<n>.js files rapidly (like every second).
The files being created are empty, and unlike the original prefs.js file have the Read-only attribute set:
https://www.screencast.com/t/l9VZrTfD6xF


Expected results:

A. Only one, writable file (e.g. prefs-1.js) is created
OR
B. No prefs-<n>.js files created at all
(Reporter)

Updated

a year ago
Component: Untriaged → Preferences
OS: Unspecified → Windows
Hardware: Unspecified → All
Component: Preferences → Preferences
Product: Firefox → Toolkit
I'm not entirely sure where the code lives that is creating these files.

@njn, I see that you have had a lot of activity in Preferences.cpp recently, as well as the annotation for pref_savePrefs (https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/modules/libpref/Preferences.cpp#708) has your name on every line :) so maybe you know who might be able to help investigate this? 

@yoric, my other guess is that this is coming from OS.File. Would you have a chance to look at this or redirect it possibly?
Flags: needinfo?(n.nethercote)
Flags: needinfo?(dteller)
(Assignee)

Comment 2

a year ago
I think this is happening in PreferencesWriter::Write() within Preferences.cpp. I'll take a look. Hopefully the fix will be as simple as checking if the file is writable.
Assignee: nobody → n.nethercote
Component: Preferences → Preferences: Backend
Flags: needinfo?(n.nethercote)
Product: Toolkit → Core
(Assignee)

Updated

a year ago
Flags: needinfo?(dteller)
(Assignee)

Comment 3

a year ago
(In reply to Nicholas Nethercote [:njn] from comment #2)
> I think this is happening in PreferencesWriter::Write() within Preferences.cpp.

That's correct. If NS_NewSafeLocalFileOutputStream() is passed a read-only file (e.g. prefs.js), it will create a temp file (e.g. prefs-1.js) and fail. This then repeats every time we try to save prefs.js.

There are two possible fixes:

- Check if the file is read-only in PreferencesWriter::Write(), and avoid the call to NS_NewSafeLocalFileOutputStream(). This is the safer option.

- Fix the underlying problem. I think this is within nsAtomicFileOutputStream::DoOpen(). This is the proper solution, but riskier because I don't understand that code well.

I've implemented both and they both fix this bug. I will post both versions.
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
glandium: as per comment 3, only one of these approaches should be used. Which patch do you like better?
Flags: needinfo?(mh+mozilla)
Attachment #8946554 - Flags: review?(mh+mozilla)
Attachment #8946555 - Flags: review?(mh+mozilla)

Comment 7

a year ago
mozreview-review
Comment on attachment 8946555 [details]
Bug 1433982 - Make nsAtomicFileOutputStream::DoOpen() fail if the file is read-only.

https://reviewboard.mozilla.org/r/216586/#review222268

::: netwerk/base/nsFileStreams.cpp:815
(Diff revision 1)
>              NS_ERROR("Can't get permissions of target file");
>              origPerm = mOpenParams.perm;
>          }
> +
> +        // Abort if |file| is not writable; it won't work as an output stream.
> +        if (!(origPerm & 0200)) {

I think doing this at this level makes more sense, but checking the permissions this way is not bullet-proof. There are many other reasons why you might not be able to write to the file that don't involve the file permissions. The most notables being ACLs and permissions on the parent directory. file->IsWritable() would be better.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8946555 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)

Comment 9

a year ago
mozreview-review
Comment on attachment 8946554 [details]
Bug 1433982 - Make nsAtomicFileOutputStream::DoOpen() fail if the file is read-only.

https://reviewboard.mozilla.org/r/216584/#review222584
Attachment #8946554 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 10

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ada933db7e2887646d64b44525da5daf5f44b85
Bug 1433982 - Make nsAtomicFileOutputStream::DoOpen() fail if the file is read-only. r=glandium

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ada933db7e2
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.