799686 - about:memory file dump is not readable on Android

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

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

Bug 788021 added code to dump about:memory info to a file. On android the file that gets written out is not generally readable by anybody other than the app itself.

That is, when I invoke nsIMemoryReporterManager.dumpMemoryReportsToFile, it writes out the file to a location like this:

10-09 16:29:16.906 E/GeckoConsole( 5353): nsIMemoryReporterManager::dumpReports() dumped reports to /data/data/org.mozilla.fennec_kats/app_tmp/memory-report-foo-5353.json.gz

The /data/data/org.mozilla.fennec_kats/app_tmp/ is world-readable, but the memory-report-foo-5353.json.gz is -rw------- and the user that owns the file is the linux user tied to the fennec application. Unless the device is rooted or the APK is debuggable, the file that gets generated cannot be read by the user.

Can we change this to either write out the file to a customizable folder, or write out the file so that it is world-readable? (Or some other solution?) I'm fine with making this an android-only change if needed.

Comment 1

[Justin Lebar (not reading bugmail)]

> Can we change this to either write out the file to a customizable folder, or write out 
> the file so that it is world-readable?

Either way would be fine with me, offhand.  Is there a canonical way to do this sort of thing on Android?

I'm currently focused on b2g at the moment, so someone else is probably going to have to do this, if you want it done anytime soon.  I'm happy to provide guidance.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: untested patch

I haven't tested it, but I think this patch will change the file so that it is world-readable.  Kats, can you try it out?

Comment 3

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

I tried it out, but it didn't work. I debugged it a bit but still not sure why it doesn't work. I traced the call to CreateUnique all the way down to the _md_iovector._open64 call (ptio.c:3569) and verified that the arguments there were sane (flags=705, mode=420 in base-10). And yet, the file is still created with 0600 permissions. I even wrote a one-line test app that calls open() with the same parameters and verified that the file it creates is 0644, as expected. So it doesn't appear to be an android bionic/kernel bug.

Also I noticed that there are two calls to CreateUnique - one for the "incomplete" temp file and then a second one for when the incomplete file is moved over to the final location. I tried changing both to 0644 but it didn't make any difference, the open call still creates the file with 0600 permissions.

I'll try investigating a little more but any ideas are welcome.

Comment 4

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

So the one-line test app that I wrote was this:

#include <fcntl.h>

int main( int argc, char** argv ) {
    return open( "/data/data/org.mozilla.fennec_kats/app_tmp/incomplete-memory-report-default-xxxxx.json.gz", 705, 420 );
}

I compile it, throw it on /data/local/, and run it using "run-as org.mozilla.fennec_kats /data/local/test". It runs, creates the file with 0644 permissions, and exits with a valid fd.

If I take the exact same line of code and put it somewhere in fennec (I put it in the AndroidBridge constructor) the file it creates has 0600 permissions. Clearly I'm missing something here, but I don't know what.

Comment 5

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

Attachment: patch that works on android

snorp pointed out that the Android zygote sets the umask to 0077 which was causing the problem. the attached modifications does the trick and creates the file with 0644 permissions.

Comment 6

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

Comment on attachment 669793 [details] [diff] [review]
untested patch

Minusing this one based on above discussion. Let me know if you'd like me to take over the bug and try-push/land the updated patch.

Comment 7

[Nicholas Nethercote [inactive]]

Please do take over!  This is off my radar now that the simple obvious fix has been shown not to work :/

Comment 8

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

Attachment: Clear umask on android

The version without the ifdefs was failing compilation on Windows, even if I moved the new #include statements into the same #if block as fcntl.h. Tryserver was not giving me any useful error messages (bug 800946), so I figured it would just be safer to ifdef the code for android.

https://tbpl.mozilla.org/?tree=Try&rev=1b9a2dd4989a
https://tbpl.mozilla.org/?tree=Try&rev=9838ceb7e0be

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 670864 [details] [diff] [review]
Clear umask on android

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

Stealing review.  Thanks for digging into this!

Comment 10

[Justin Lebar (not reading bugmail)]

Thanks for doing the review!

Comment 11

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f47eacb16e3

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f47eacb16e3

Comment 13

[Justin Lebar (not reading bugmail)]

Now that I'm looking at this patch, it's problematic for two reasons:

1) It introduces a race condition across threads with setting and resetting umask().  Unless we always create files on the main thread (unlikely!), this is bad.

2) More importantly, it fixes only one instance of CreateUnique.  This means that I have to copy-paste this goop to every call I make to CreateUnique (e.g. in bug 800486), if that caller wants the file to be globally-readable.

It seems to me that the correct solution would be either to modify Android's implementation of CreateUnique to respect the given permissions mask (which would fix (2) but not (1)), or to umask(0) Firefox on Android when it starts up.

Either way, I'm not going to include this fix in my patch to bug 800486, because it's not the correct way of doing things.  I'd like us to remove it from the memory-reporting code as soon as we can.

Comment 14

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

Good points. I'm fine with calling umask(0) on startup, but I don't know exactly where in the startup code path we should be calling it. Any suggestions on that?

Also, there are a few other pieces of code that do this set/reset of umask [1] that presumably are subject to the same problem. Do those need to be changed as well?

[1] http://mxr.mozilla.org/mozilla-central/ident?i=umask

Comment 15

[Justin Lebar (not reading bugmail)]

> Do those need to be changed as well?

They don't seem to be Android-specific, so maybe it's not your problem.

Also, with the exception of sqlite and updater, they appear to be calling umask only for the purposes of getting the umask, not setting it.  Which is still racy, but :shrug:.

> I'm fine with calling umask(0) on startup, but I don't know exactly where in the startup code path 
> we should be calling it. Any suggestions on that?

Assuming it's a sane thing to do (you should check with a 'nix expert like glandium), maybe you could do it in widget/android/nsAppShell::Init().  But there's probably a better place to put it; I'm not expert here either.

Comment 16

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

CC'ing glandium - any thoughts on comments #13 - #15? Note that instead of doing umask(0) we could do something safer like umask(0022) if that helps.

Comment 17

[Mike Hommey [:glandium]]

Changing the umask is possibly dangerous. I'd very much prefer if you'd just chmod() the file you want to ensure access to.