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

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: 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.

Comment 1

[Justin Lebar (not reading bugmail)]

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.

Comment 2

[Justin Lebar (not reading bugmail)]

> 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.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Move chmod into CreateAndKeepOpen

Moving to CreateAndKeepOpen which feels more appropriate to me.

Comment 5

[Mike Hommey [:glandium]]

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 6

[Mike Hommey [:glandium]]

Comment on attachment 676383 [details] [diff] [review]
Move chmod into CreateAndKeepOpen

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

Per comment 5

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

[Mike Hommey [:glandium]]

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

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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])?

Comment 10

[Mike Hommey [:glandium]]

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

Comment 11

[Justin Lebar (not reading bugmail)]

> 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 12

[Justin Lebar (not reading bugmail)]

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!

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Use chmod instead of umask (v3)

Back to the first patch, but updating nsCycleCollector as well.

Comment 14

[Mike Hommey [:glandium]]

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?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(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

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4f1f111f4cd5