Closed
Bug 879489
Opened 12 years ago
Closed 12 years ago
Make sure /data/local/tmp is a directory and is writable
Categories
(Testing Graveyard :: SUTAgent, defect)
Testing Graveyard
SUTAgent
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla24
People
(Reporter: bc, Assigned: bc)
Details
Attachments
(1 file, 1 obsolete file)
6.19 KB,
patch
|
jmaher
:
review+
|
Details | Diff | 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)
![]() |
||
Comment 1•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 years ago
|
Assignee: nobody → bclary
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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•12 years ago
|
||
gbrown, wlach: any objections?
Flags: needinfo?(wlachance)
Flags: needinfo?(gbrown)
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0327342bd9dc
thanks everyone.
Comment 10•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•7 years ago
|
Product: Testing → Testing Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•