Hadoop Common
  1. Hadoop Common
  2. HADOOP-5462

Glibc double free exception thrown when chown syscall fails.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.21.0
    • Fix Version/s: 0.21.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      When setuid script's chown call fails, a glibc double free exception is thrown. The reason for this is that file_handle which was opened to write the pid file is already closed and the cleanup: label tries to close it once again.

      1. HADOOP-5462.patch
        2 kB
        Sreekanth Ramakrishnan
      2. hadoop-5462-1.patch
        6 kB
        Sreekanth Ramakrishnan
      3. hadoop-5462-2.patch
        7 kB
        Sreekanth Ramakrishnan

        Activity

        Hide
        Sreekanth Ramakrishnan added a comment -

        Attaching patch which fixes this issue.

        Show
        Sreekanth Ramakrishnan added a comment - Attaching patch which fixes this issue.
        Hide
        Hemanth Yamijala added a comment -

        When setuid script's chown call fails ...

        This could happen if deployment of the setuid executable is not done correctly as we discovered during a deployment exercise.

        Show
        Hemanth Yamijala added a comment - When setuid script's chown call fails ... This could happen if deployment of the setuid executable is not done correctly as we discovered during a deployment exercise.
        Hide
        Hemanth Yamijala added a comment -

        Few comments on the patch:

        • run_task_as_user: variable task_script is unused.
        • run_task_as_user: when the pid_path variable is freed, it would be good to set it to NULL. Same holds for task_script_path if execlp fails.
        • run_task_as_user: Regarding the removal of chown, though the call is not necessary, I am thinking a better idea is to actually do this after switching the user. I don't see why root privileges are required to do this operation. This way, the file would be written as owned by the child, with the right permissions (so, no need for the chmod as well).
        • kill_user_task: Like above, I think we can have the reading of the pid file happen after switching the user. Coupled with that change, since the file will be written as the user itself, it can be read back by him as well.
        • run_task_as_user: We are doing an fclose of LOGFILE (both explicitly and by the fcloseall call) before execlp, but afterwards, we are writing to it in some error conditions. Maybe we can just remove these prints. Coupled with the above changes to do more operations after switching the user, it may mean that we cannot log in even more cases. Because the error codes are indicative enough, I think this is fine though.
        Show
        Hemanth Yamijala added a comment - Few comments on the patch: run_task_as_user: variable task_script is unused. run_task_as_user: when the pid_path variable is freed, it would be good to set it to NULL. Same holds for task_script_path if execlp fails. run_task_as_user: Regarding the removal of chown, though the call is not necessary, I am thinking a better idea is to actually do this after switching the user. I don't see why root privileges are required to do this operation. This way, the file would be written as owned by the child, with the right permissions (so, no need for the chmod as well). kill_user_task: Like above, I think we can have the reading of the pid file happen after switching the user. Coupled with that change, since the file will be written as the user itself, it can be read back by him as well. run_task_as_user: We are doing an fclose of LOGFILE (both explicitly and by the fcloseall call) before execlp, but afterwards, we are writing to it in some error conditions. Maybe we can just remove these prints. Coupled with the above changes to do more operations after switching the user, it may mean that we cannot log in even more cases. Because the error codes are indicative enough, I think this is fine though.
        Hide
        Sreekanth Ramakrishnan added a comment -

        Removed chown and chmod calls in the run_task_as_user.

        Changed the way binary switches user, now binary immediately switches to the passed user and then writes out the .pid file, so chown and chmod calls is no longer required.

        Removed fprintf statements which were called after switching the user. Removed appropriate error codes.

        Show
        Sreekanth Ramakrishnan added a comment - Removed chown and chmod calls in the run_task_as_user. Changed the way binary switches user, now binary immediately switches to the passed user and then writes out the .pid file, so chown and chmod calls is no longer required. Removed fprintf statements which were called after switching the user. Removed appropriate error codes.
        Hide
        Hemanth Yamijala added a comment -

        Looking good.

        Minor points:

        I think the setuid exe should work if the task-controller.cfg file is readable only by root. Hence, it makes sense to change the user after loading the configuration which happens after the first call to check_tt_root.
        Another problem with the current code is that if the setuid operation fails, then we are freeing up the configuration. But the config struct is not memset to 0. Hence it might have garbage values and fail. I think it is better to initialize this to a zero'ed structure.

        Show
        Hemanth Yamijala added a comment - Looking good. Minor points: I think the setuid exe should work if the task-controller.cfg file is readable only by root. Hence, it makes sense to change the user after loading the configuration which happens after the first call to check_tt_root. Another problem with the current code is that if the setuid operation fails, then we are freeing up the configuration. But the config struct is not memset to 0. Hence it might have garbage values and fail. I think it is better to initialize this to a zero'ed structure.
        Hide
        Hemanth Yamijala added a comment -

        After the above comments are implemented, I think it is good to go. So make this patch available after that.

        Show
        Hemanth Yamijala added a comment - After the above comments are implemented, I think it is good to go. So make this patch available after that.
        Hide
        Sreekanth Ramakrishnan added a comment -

        Incorporated Hemanths comment:

        • Moved TT root checking before switching user.
        • Initialized the structure in configuration.c so that values are initalized properly.
        Show
        Sreekanth Ramakrishnan added a comment - Incorporated Hemanths comment: Moved TT root checking before switching user. Initialized the structure in configuration.c so that values are initalized properly.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12404806/hadoop-5462-2.patch
        against trunk revision 762885.

        +1 @author. The patch does not contain any @author tags.

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +1 javadoc. The javadoc tool did not generate any warning messages.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 findbugs. The patch does not introduce any new Findbugs warnings.

        -1 Eclipse classpath. The patch causes the Eclipse classpath to differ from the contents of the lib directories.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        -1 core tests. The patch failed core unit tests.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/160/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/160/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/160/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/160/console

        This message is automatically generated.

        Show
        Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12404806/hadoop-5462-2.patch against trunk revision 762885. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. -1 Eclipse classpath. The patch causes the Eclipse classpath to differ from the contents of the lib directories. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/160/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/160/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/160/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/160/console This message is automatically generated.
        Hide
        Hemanth Yamijala added a comment -

        The core test failures are not related. I just committed this to trunk. Thanks, Sreekanth !

        Show
        Hemanth Yamijala added a comment - The core test failures are not related. I just committed this to trunk. Thanks, Sreekanth !

          People

          • Assignee:
            Sreekanth Ramakrishnan
            Reporter:
            Sreekanth Ramakrishnan
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development