879489 - Make sure /data/local/tmp is a directory and is writable

Status: RESOLVED FIXED

(Testing Graveyard :: SUTAgent, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: patch v1

On galaxy s3's especially, /data/local/tmp's permissions are reset on reboot, which means the SUTAgent may not have write permission to the directory. On other devices it is possible to muck with /data/local/tmp and create it as a file.

This patch:

* makes sure everyone can read, write, execute (list) /data/local so we are sure to be able to delete and recreate /data/local/tmp if needed.

* makes sure /data/local/tmp is a directory and if not deletes and recreates it.

* makes sure everyone can read, write, execute /data/local/tmp. Note I don't restrict the ChmodDir call to the case where /data/local/tmp is not writable by the SUTAgent, since it is possible that it may have been created where the SUTAgent can read, write, execute it, but no one else can.

* makes sure ChmodDir executes chmod as root so we don't fail due to permission problems.

Comment 1

[Geoff Brown [:gbrown]]

I like this idea -- I have seen this problem too.

I am a little uneasy about implementing it in GetTestRoot; I would prefer to see a new sut command invoked explicitly from devicemanagerSUT initialization, or something like that.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 758185 [details] [diff] [review]
patch v1

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

I tend to lean more towards :gbrown and his assessment of making this not part of getTestRoot, but part of a separate command/function.  If we were to leave this in here, I would check for the media first, then if that fails, go through the routine of making /data/local/tmp writeable.

::: build/mobile/sutagent/android/DoCommand.java
@@ +1358,5 @@
> +        String chmodResult;
> +        File localDir = new java.io.File("/data/local");
> +        if (!localDir.canWrite()) {
> +            chmodResult = ChmodDir("/data/local");
> +            Log.i("SUTAgentAndroid", chmodResult);

this log message has no context.  Maybe indicate "changed permissions on /data/local to make it writable: " +chmodResult

@@ +1364,5 @@
> +        File tmpDir = new java.io.File("/data/local/tmp");
> +        if (tmpDir.exists() && !tmpDir.isDirectory()) {
> +            if (!tmpDir.delete()) {
> +                Log.e("SUTAgentAndroid", "Could not delete directory /data/local/tmp");
> +            }

I don't understand why we try to delete this.  I could see somebody wanting to store data there across runs or reboots.

@@ +1370,5 @@
> +        if (!tmpDir.exists() && !tmpDir.mkdirs()) {
> +            Log.e("SUTAgentAndroid", "Could not create directory /data/local/tmp");
> +        }
> +        chmodResult = ChmodDir("/data/local/tmp");
> +        Log.i("SUTAgentAndroid", chmodResult);

again, this could use some context.

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 758185 [details] [diff] [review]
> patch v1
> 
> Review of attachment 758185 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I tend to lean more towards :gbrown and his assessment of making this not
> part of getTestRoot, but part of a separate command/function.  If we were to
> leave this in here, I would check for the media first, then if that fails,
> go through the routine of making /data/local/tmp writeable.
> 

I was thinking the best place would be in onCreate so that it would be done automatically before anything tried to access /data/local/tmp.

Checking if /data/local/tmp is writable by the SUTAgent alone isn't sufficient since it can be writable by the SUTAgent but not by anyone else. I think we should always make it writable by everyone.

> ::: build/mobile/sutagent/android/DoCommand.java
> @@ +1358,5 @@
> > +        String chmodResult;
> > +        File localDir = new java.io.File("/data/local");
> > +        if (!localDir.canWrite()) {
> > +            chmodResult = ChmodDir("/data/local");
> > +            Log.i("SUTAgentAndroid", chmodResult);
> 
> this log message has no context.  Maybe indicate "changed permissions on
> /data/local to make it writable: " +chmodResult
> 

Ok.

> @@ +1364,5 @@
> > +        File tmpDir = new java.io.File("/data/local/tmp");
> > +        if (tmpDir.exists() && !tmpDir.isDirectory()) {
> > +            if (!tmpDir.delete()) {
> > +                Log.e("SUTAgentAndroid", "Could not delete directory /data/local/tmp");
> > +            }
> 
> I don't understand why we try to delete this.  I could see somebody wanting
> to store data there across runs or reboots.
> 

Because it is a file and not a directory. If someone mucked it up (I have at least once) and created it as a file, we would need to get rid of it altogether and recreate it as a directory.

> @@ +1370,5 @@
> > +        if (!tmpDir.exists() && !tmpDir.mkdirs()) {
> > +            Log.e("SUTAgentAndroid", "Could not create directory /data/local/tmp");
> > +        }
> > +        chmodResult = ChmodDir("/data/local/tmp");
> > +        Log.i("SUTAgentAndroid", chmodResult);
> 
> again, this could use some context.

Ok.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: patch v2

This patch adds a method FixDataLocalPermissions to DoCommand and calls it from the SUTAgent's onCreate.

It adds the context to the log messages but that is somewhat redundant considering the result of Chmod, .e.g.

06-05 07:44:48.503  2188  2188 I SUTAgentAndroid: Changed permissions on /data/local/tmp to make it writable: Changing permissions for /data/local/tmp

In GetTestRoot, I moved the creation of /data/local/tmp/tests to after the check for the external storage since it isn't used if the external storage is available and also delete it before returning.

You can download SUTAgent built with this patch at: http://people.mozilla.org/~bclary/sutAgentAndroid.apk

gbrown, wlach: Do you see any problems with these changes?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 758597 [details] [diff] [review]
patch v2

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

I like this now!

Comment 6

[Bob Clary [:bc] (inactive)]

gbrown, wlach: any objections?

Comment 7

[Geoff Brown [:gbrown]]

That looks fine to me - thanks!

Comment 8

[William Lachance (:wlach)]

This looks ok to me.

Comment 9

[Bob Clary [:bc] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0327342bd9dc

thanks everyone.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0327342bd9dc