Closed Bug 1433982 Opened 3 years ago Closed 3 years ago

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


(Core :: Preferences: Backend, defect)

57 Branch
Not set



Tracking Status
firefox60 --- fixed


(Reporter: nikolay.khil, Assigned: njn)



(1 file, 1 obsolete file)

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

Example of Deny ACE:

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:

Expected results:

A. Only one, writable file (e.g. prefs-1.js) is created
B. No prefs-<n>.js files created at all
Component: Untriaged → Preferences
OS: Unspecified → Windows
Hardware: Unspecified → All
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 ( 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)
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
Flags: needinfo?(dteller)
(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.
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 on attachment 8946555 [details]
Bug 1433982 - Make nsAtomicFileOutputStream::DoOpen() fail if the file is read-only.

::: 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.
Attachment #8946555 - Attachment is obsolete: true
Flags: needinfo?(mh+mozilla)
Comment on attachment 8946554 [details]
Bug 1433982 - Make nsAtomicFileOutputStream::DoOpen() fail if the file is read-only.
Attachment #8946554 - Flags: review?(mh+mozilla) → review+
Bug 1433982 - Make nsAtomicFileOutputStream::DoOpen() fail if the file is read-only. r=glandium
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.