Don't use umask fiddling to make about:memory dumps readable on android

RESOLVED FIXED in mozilla19

Status

()

Toolkit
about:memory
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

unspecified
mozilla19
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 676247 [details] [diff] [review]
Use chmod instead of umask

See comments 13-17 on bug 799686. The attached patch removes the umask fiddling and instead calls chmod to make the about:memory dump readable.
Attachment #676247 - Flags: review?(mh+mozilla)
Attachment #676247 - Flags: review?(justin.lebar+bug)
Would it make sense to put this in the Create/CreateUnique implementation?  Otherwise we have to copy/paste this code at least into the gc/cc log creation code.
> Otherwise we have to copy/paste this code at least into the gc/cc log creation code.

See call to CreateUnique in xpcom/base/nsCycleCollector.cpp.
(In reply to Justin Lebar [:jlebar] from comment #1)
> Would it make sense to put this in the Create/CreateUnique implementation? 

I do kind of agree, but every time I try to put it there I feel it needs to go even deeper. After staring at it some more I think maybe the bottom of this function is probably the most appropriate place: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#435

:glandium, if you have any thoughts as to where this code belongs please let me know.
Created attachment 676383 [details] [diff] [review]
Move chmod into CreateAndKeepOpen

Moving to CreateAndKeepOpen which feels more appropriate to me.
Attachment #676247 - Attachment is obsolete: true
Attachment #676247 - Flags: review?(mh+mozilla)
Attachment #676247 - Flags: review?(justin.lebar+bug)
Attachment #676383 - Flags: review?(mh+mozilla)
Attachment #676383 - Flags: review?(justin.lebar+bug)
It actually doesn't feel right to do it at the lowest level. That ends up being like setting umask at startup. And I don't think we want all files to be treated this way, and to expose most of the user profile that way.
Comment on attachment 676383 [details] [diff] [review]
Move chmod into CreateAndKeepOpen

Review of attachment 676383 [details] [diff] [review]:
-----------------------------------------------------------------

Per comment 5
Attachment #676383 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #5)
> It actually doesn't feel right to do it at the lowest level. That ends up
> being like setting umask at startup. And I don't think we want all files to
> be treated this way, and to expose most of the user profile that way.

Note that the files will only be exposed if the code that calls create specifies that the file should be exposed. Most file creation code specifies 0600 as the permissions on the file, so it's not an issue. To be sure, I checked the file permissions on an "old" profile (without this change) as well as an on a "new" profile (created by a build with this change). Of the files that were in both old and new, I only found one instance where the permissions of a file in the profile was different - "localstore.rdf" now has 0666 permissions instead of the 0600 it had originally.
(In reply to Kartikaya Gupta (:kats) from comment #7)
> I only found one instance where the
> permissions of a file in the profile was different - "localstore.rdf" now
> has 0666 permissions instead of the 0600 it had originally.

That seems a pretty compelling reason to avoid it, or at the very very least not to use the requested permissions as-is.
The localstore.rdf file appears to be created at http://mxr.mozilla.org/mozilla-central/source/rdf/datasource/src/nsLocalStore.cpp#335 with permissions explicitly set to 0666, so to me this seems like intended behaviour.

If you still disagree, what is your suggested solution? Do you want me to & the permissions with 0744 so files are only readable and not writeable? Or do you prefer the original patch I had (which is the now-obsoleted attachment #676247 [details] [diff] [review])?
(In reply to Kartikaya Gupta (:kats) from comment #9)
> The localstore.rdf file appears to be created at
> http://mxr.mozilla.org/mozilla-central/source/rdf/datasource/src/
> nsLocalStore.cpp#335 with permissions explicitly set to 0666, so to me this
> seems like intended behaviour.

Certainly, the intended behaviour is that the umask does its job on these permissions. So that if people use e.g. a 0770 umask, the file properly has the write bit for the group (which certainly is what users would expect from setting such a umask)

> If you still disagree, what is your suggested solution? Do you want me to &
> the permissions with 0744 so files are only readable and not writeable? Or
> do you prefer the original patch I had (which is the now-obsoleted
> attachment #676247 [details] [diff] [review] [diff] [review])?

I'd go for the original patch, although you should probably still create the file with 0644 by default and chmod if it doesn't have those bits on android.

Or, save the file in /sdcard and don't care about permissions, since files created in /sdcard are always readable by all processes.
> I'd go for the original patch, although you should probably still create the file with 
> 0644 by default and chmod if it doesn't have those bits on android.

So every time I CreateUnique() I have to add an ifdef android guard if I want that file to be readable by non-root?

I guess I only have two instances of this right now, so I can complain about this when I have more.
Comment on attachment 676383 [details] [diff] [review]
Move chmod into CreateAndKeepOpen

Whatever glandium wants to do here is fine with me.  My tugging you in a separate direction is not helpful here!
Attachment #676383 - Flags: review?(justin.lebar+bug)
Created attachment 676630 [details] [diff] [review]
Use chmod instead of umask (v3)

Back to the first patch, but updating nsCycleCollector as well.
Attachment #676383 - Attachment is obsolete: true
Attachment #676630 - Flags: review?(mh+mozilla)
Comment on attachment 676630 [details] [diff] [review]
Use chmod instead of umask (v3)

Review of attachment 676630 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsCycleCollector.cpp
@@ +133,5 @@
>  #include <process.h>
>  #endif
> +#ifdef ANDROID
> +#include <sys/stat.h>
> +#include <sys/types.h>

Is sys/types.h needed?
Attachment #676630 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #14)
> Is sys/types.h needed?

Good point. Removed (from both files) and landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f1f111f4cd5
https://hg.mozilla.org/mozilla-central/rev/4f1f111f4cd5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.