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

RESOLVED FIXED in mozilla24

Status

RESOLVED FIXED
6 years ago
9 months ago

People

(Reporter: bc, Assigned: bc)

Tracking

Trunk
mozilla24

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 758185 [details] [diff] [review]
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.
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-
(Assignee)

Comment 3

6 years ago
(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)

Updated

6 years ago
Assignee: nobody → bclary
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 years ago
Created attachment 758597 [details] [diff] [review]
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?
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+
(Assignee)

Comment 6

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

9 months ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.