hadoop-mapreduce-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hemanth Yamijala (JIRA)" <j...@apache.org>
Subject [jira] Commented: (MAPREDUCE-896) Users can set non-writable permissions on temporary files for TT and can abuse disk usage.
Date Sun, 06 Dec 2009 06:37:20 GMT

    [ https://issues.apache.org/jira/browse/MAPREDUCE-896?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12786575#action_12786575
] 

Hemanth Yamijala commented on MAPREDUCE-896:
--------------------------------------------

I looked at the Y! 20 patch. Some comments:

- TaskTracker.buildPathForDeletion need not be public.
- Was there a need to change CleanupQueue.addToQueue to take a FileSystem as argument instead
of Configuration ? It has caused more changes than required by this patch - like in JobTracker
and JobInProgress. Can we retain the original API and pass in a Configuration as before ?

- When adding a task directory to delete, we are adding paths from all the local directories
instead of just the one where files for the task are actually created. At a minimum, this
is more work being done than necessary. More importantly, I don't know if there are any side
effects this will cause. We can check which among the local directories the path belongs to
(by doing a startsWith on the path) and all only that I think.
- Shouldn't getLocalDirs take the tasktracker's configuration always ? In which case, it doesn't
need to take the JobConf as a parameter, but can use fConf.
- The log statements in CleanupQueue.PathCleanupThread.run are printing context.path which
will only be the mapred local dir. We actually need the full path, otherwise the log statements
could be confusing. Indeed, I would suggest a slight refactoring of the PathDeletionContext
class, because as it exists currently we have one mode or the other that works - either a
fullPath is provided or the path is built from other bits of data - like jobId, taskId etc.
So, I would suggest something along the following lines:
{code}
class PathDeletionContext {
  String fullPath;
  FileSystem fs;

  protected String getPathForDeletion() {
    return fullPath;
  }

  protected void enablePathForCleanup() {
    // do nothing
  }
}

class TaskControllerPathDeletionContext extends PathDeletionContext {
  String user;
  String jobId;
  String taskId;
  boolean isCleanupTask;
  boolean isWorkDir;
  TaskController taskController;
  Path p;

  @Override
  protected String getPathForDeletion() {
    TaskTracker.buildPathForDeletion(this);
  }

  @Override
  protected void enablePathForCleanup() {
    taskController.enablePathForCleanup(this);
  }
}
{code}

Then we can use PathDeletionContext in all cases where we don't need to use the taskController
and the sub-class in other cases. CleanupQueue will naturally take and store PathDeletionContext
objects. getPathForDeletion can be called to get the final path for deletion. I feel this
design is cleaner. Thoughts ?
- DefaultTaskController.enableTaskForCleanup should be package private.
- In other APIs of LinuxTaskController - like buildLaunchTaskArgs, we find out if the task
is a cleanup task and adjust paths directly. I think we can do the same thing for the new
command also. This is not less secure, because we are still constructing the full path from
the command args, but we abstract the task-controller from details like cleanup task. It is
less clear whether the same thing should be done for workDir (i.e. should we append that to
taskid in LinuxTaskController itself.) For that we may need a flag still, but I am OK if that
is also resolved in LinuxTaskController itself and we completely eliminate flags to pass to
task-controller.
- The List of args in buildChangePathPermissionsArgs should be of the right size. (It's not
5). Also, I think it is useful to retain the order of commands the same. i.e. Let the mapred
local dir be the first argument, then job-id, then task-id.
- I think we must allocate the exact amount of memory required in build_dir_path. This can
be done by defining a format string like TT_LOCAL_TASK_SCRIPT_PATTERN and then summing the
lengths of this string, and the arguments like jobid, taskid, mapred local dir etc. Then we
can use snprintf to build the path instead of multiple (unsafe) strcat and strcpy calls. Again,
please look at get_task_file_path for an example.
- Return values of calls like malloc should all be checked. When this is done, calls to build_dir_path
can fail, which must also be checked.
- In TaskRunner.deleteDirContents, I think if we get an InterruptedException, we should return
immediately. Because otherwise, the operation is not really interrupted and it can get stuck
permanently.
- The intent of the testcase in TestChildTaskDirs is nice. But I am worried that since directory
cleanup happens asynchronously, this might fail due to timing issues (like the TODO in the
comment says). One option could be to use an inline directory cleaner. Can we try that ? 
- Should we also verify that the taskattemptdir is also cleaned up ?
- There are some TODOs in the tests, can you please remove them after addressing the concerns
?


> Users can set non-writable permissions on temporary files for TT and can abuse disk usage.
> ------------------------------------------------------------------------------------------
>
>                 Key: MAPREDUCE-896
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-896
>             Project: Hadoop Map/Reduce
>          Issue Type: Bug
>          Components: tasktracker
>    Affects Versions: 0.21.0
>            Reporter: Vinod K V
>            Assignee: Ravi Gummadi
>             Fix For: 0.21.0
>
>         Attachments: MR-896.patch, MR-896.v1.patch, y896.v1.patch
>
>
> As of now, irrespective of the TaskController in use, TT itself does a full delete on
local files created by itself or job tasks. This step, depending upon TT's umask and the permissions
set by files by the user, for e.g in job-work/task-work or child.tmp directories, may or may
not go through successful completion fully. Thus is left an opportunity for abusing disk space
usage either accidentally or intentionally by TT/users.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message