Closed Bug 879489 Opened 8 years ago Closed 8 years ago

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

Categories

(Testing Graveyard :: SUTAgent, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla24

People

(Reporter: bc, Assigned: bc)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #758185 - Flags: review?(jmaher)
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 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.
Attachment #758185 - Flags: review?(jmaher) → review-
(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.
Assignee: nobody → bclary
Status: NEW → ASSIGNED
Attached patch patch v2Splinter Review
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?
Attachment #758185 - Attachment is obsolete: true
Attachment #758597 - Flags: review?(jmaher)
Comment on attachment 758597 [details] [diff] [review]
patch v2

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

I like this now!
Attachment #758597 - Flags: review?(jmaher) → review+
gbrown, wlach: any objections?
Flags: needinfo?(wlachance)
Flags: needinfo?(gbrown)
That looks fine to me - thanks!
Flags: needinfo?(gbrown)
This looks ok to me.
Flags: needinfo?(wlachance)
https://hg.mozilla.org/mozilla-central/rev/0327342bd9dc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.