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

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Nikolay Khil]

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

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Nicholas Nethercote [inactive]]

(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 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1433982 - Make nsAtomicFileOutputStream::DoOpen() fail if the file is read-only.

Review commit: https://reviewboard.mozilla.org/r/216584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216584/

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1433982 - Make nsAtomicFileOutputStream::DoOpen() fail if the file is read-only.

This means we don't leave behind prefs-<n>.js files when prefs.js is read-only.

Review commit: https://reviewboard.mozilla.org/r/216586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216586/

Comment 6

[Nicholas Nethercote [inactive]]

glandium: as per comment 3, only one of these approaches should be used. Which patch do you like better?

Comment 7

[Mike Hommey [:glandium]]

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 8

[Nicholas Nethercote [inactive]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216584/diff/1-2/

Comment 9

[Mike Hommey [:glandium]]

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

https://reviewboard.mozilla.org/r/216584/#review222584

Comment 10

[Nicholas Nethercote [inactive]]

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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5ada933db7e2