Skip to content

Cleanup the os_dirpath logic #2756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 20, 2017
Merged

Cleanup the os_dirpath logic #2756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c722fb9
Cleanup the os_dirpath logic so it doesn't error out if the directory…
Jan 18, 2017
867c841
Quick fix: save the errno from the mkdir call as the call to stat wil…
Jan 18, 2017
de539e0
opal/util: fix a race condition in opal_os_dirpath_create()
ggouaillardet Jan 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions opal/util/help-opal-util.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# -*- text -*-
#
# Copyright (c) 2009 Cisco Systems, Inc. All rights reserved.
# Copyright (c) 2017 Intel, Inc. All rights reserved.
# $COPYRIGHT$
#
# Additional copyrights may follow
Expand Down Expand Up @@ -93,3 +94,23 @@ resource:
Limit: %s

Please correct the request and try again.
#
[dir-mode]
While working through a directory tree, we were unable to set
a directory to the desired mode:

Directory: %s
Mode: %0x
Error: %s

Please check to ensure you have adequate permissions to perform
the desired operation.
#
[mkdir-failed]
A call to mkdir was unable to create the desired directory:

Directory: %s
Error: %s

Please check to ensure you have adequate permissions to perform
the desired operation.
57 changes: 30 additions & 27 deletions opal/util/os_dirpath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
* University of Stuttgart. All rights reserved.
* Copyright (c) 2004-2005 The Regents of the University of California.
* All rights reserved.
* Copyright (c) 2015 Research Organization for Information Science
* Copyright (c) 2015-2017 Research Organization for Information Science
* and Technology (RIST). All rights reserved.
* Copyright (c) 2016-2017 Intel, Inc. All rights reserved.
* $COPYRIGHT$
*
* Additional copyrights may follow
Expand Down Expand Up @@ -39,6 +40,7 @@

#include "opal/util/output.h"
#include "opal/util/os_dirpath.h"
#include "opal/util/show_help.h"
#include "opal/util/argv.h"
#include "opal/util/os_path.h"
#include "opal/constants.h"
Expand All @@ -63,11 +65,9 @@ int opal_os_dirpath_create(const char *path, const mode_t mode)
if (0 == (ret = chmod(path, (buf.st_mode | mode)))) { /* successfully change mode */
return(OPAL_SUCCESS);
}
opal_output(0,
"opal_os_dirpath_create: "
"Error: Unable to create directory (%s), unable to set the correct mode [%d]\n",
path, ret);
return(OPAL_ERROR); /* can't set correct mode */
opal_show_help("help-opal-util.txt", "dir-mode", true,
path, mode, strerror(errno));
return(OPAL_ERR_PERM); /* can't set correct mode */
}

/* quick -- try to make directory */
Expand Down Expand Up @@ -112,17 +112,21 @@ int opal_os_dirpath_create(const char *path, const mode_t mode)
strcat(tmp, parts[i]);
}

/* Now that we finally have the name to check, check it.
Create it if it doesn't exist. */
ret = mkdir(tmp, mode);
if ((0 > ret && EEXIST != errno) || 0 != stat(tmp, &buf)) {
opal_output(0,
"opal_os_dirpath_create: "
"Error: Unable to create the sub-directory (%s) of (%s), mkdir failed [%d]\n",
tmp, path, ret);
/* Now that we have the name, try to create it */
mkdir(tmp, mode);
ret = errno; // save the errno for an error msg, if needed
if (0 != stat(tmp, &buf)) {
Copy link
Contributor

@ggouaillardet ggouaillardet Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the pointed commit was meant to solve a race condition (e.g. mkdir() after stat())

there still is a tricky race condition here ... someone else might have mkdir() but with the wrong permissions, so it will likely crash later.
so i guess we have to check the permissions at the end.

this is for master

diff --git a/opal/util/os_dirpath.c b/opal/util/os_dirpath.c
index 9ef9ebb..2328179 100644
--- a/opal/util/os_dirpath.c
+++ b/opal/util/os_dirpath.c
@@ -9,7 +9,7 @@
  *                         University of Stuttgart.  All rights reserved.
  * Copyright (c) 2004-2005 The Regents of the University of California.
  *                         All rights reserved.
- * Copyright (c) 2015-2016 Research Organization for Information Science
+ * Copyright (c) 2015-2017 Research Organization for Information Science
  *                         and Technology (RIST). All rights reserved.
  * Copyright (c) 2016-2017 Intel, Inc. All rights reserved.
  * $COPYRIGHT$
@@ -121,6 +121,12 @@ int opal_os_dirpath_create(const char *path, const mode_t mode)
             opal_argv_free(parts);
             free(tmp);
             return OPAL_ERROR;
+        } else if (i == (len-1) && (mode != (mode & buf.st_mode)) && (0 > chmod(tmp, (buf.st_mode | mode)))) {
+            opal_show_help("help-opal-util.txt", "dir-mode", true,
+                           tmp, mode, strerror(errno));
+            opal_argv_free(parts);
+            free(tmp);
+            return(OPAL_ERR_PERM); /* can't set correct mode */
         }
     }

Copy link
Contributor Author

@rhc54 rhc54 Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds reasonable - if you want to commit to master, I'll cherry-pick it into this PR

Copy link
Contributor

@ggouaillardet ggouaillardet Jan 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i made #2759
will merge when CI completes

opal_show_help("help-opal-util.txt", "mkdir-failed", true,
tmp, strerror(ret));
opal_argv_free(parts);
free(tmp);
return OPAL_ERROR;
} else if (i == (len-1) && (mode != (mode & buf.st_mode)) && (0 > chmod(tmp, (buf.st_mode | mode)))) {
opal_show_help("help-opal-util.txt", "dir-mode", true,
tmp, mode, strerror(errno));
opal_argv_free(parts);
free(tmp);
return(OPAL_ERR_PERM); /* can't set correct mode */
}
}

Expand Down Expand Up @@ -263,19 +267,19 @@ bool opal_os_dirpath_is_empty(const char *path ) {
struct dirent *ep;

if (NULL != path) { /* protect against error */
dp = opendir(path);
if (NULL != dp) {
while ((ep = readdir(dp))) {
if ((0 != strcmp(ep->d_name, ".")) &&
(0 != strcmp(ep->d_name, ".."))) {
dp = opendir(path);
if (NULL != dp) {
while ((ep = readdir(dp))) {
if ((0 != strcmp(ep->d_name, ".")) &&
(0 != strcmp(ep->d_name, ".."))) {
closedir(dp);
return false;
}
}
closedir(dp);
return true;
}
return false;
return false;
}
}
closedir(dp);
return true;
}
return false;
}

return true;
Expand Down Expand Up @@ -306,4 +310,3 @@ int opal_os_dirpath_access(const char *path, const mode_t in_mode ) {
return( OPAL_ERR_NOT_FOUND );
}
}