avoid race conditions when creating directories

[jhpalmieri]

It is much better to create directories using

try:
    os.makedirs(PATH)
except OSError....
    # ignore errors if PATH already exists, deal with other errors
    ...

than

if not os.path.isdir(PATH):
    os.makedirs(PATH)

Doing it the first way avoids race conditions. Doing it the second way can lead to security holes, at least in theory. The attached patch changes many of the directory-creating code in Sage to do it the first way.

Apply trac_11972-mkdir.patch and 11972_review.patch

[jhpalmieri]

The attached patch should take care of most of the places where Sage creates directories on start-up. The exceptions (as far as I can tell) are .sage/ipython and .sage/matplotlib-VERSION. The second of these is created somewhere in the matplotlib code, and I believe that the code is written or patched to avoid race conditions, just like the patch here. I don't know about the ipython directory.

[leif]

If the code does not behave differently in case a directory doesn't yet exist, we can simply call a shell script which does

for dir in $DIRS; do
    mkdir -p "$dir" || return 1
done
return 0

instead of calling sage-starts before running any tests, not just sage-ptest (sage -tp ..., make ptest*).

Of course fixing present code isn't bad either; we could actually do both (and prominently document what should be performed whenever new directories -- similar applies to other files -- are created).

[jdemeyer]

This patch is a very good idea. But wouldn't the following be better:

    try:
        os.makedirs(dir)
    except OSError:
        if not os.path.isdir(dir):
            raise

Also, it would be nice to have a failing example, for example

sage: sage_executable = os.path.join(SAGE_ROOT, 'sage')
sage: sage_makedirs(sage_executable)
Traceback...

[jhpalmieri]

Here's a new patch. The difference between the old and new:

sage/misc/misc.py

@@ -55 +55 @@
 
     EXAMPLES::
 
-        sage: sage_makedirs(DOT_SAGE)
+        sage: from sage.misc.misc import sage_makedirs
+        sage: sage_makedirs(DOT_SAGE) # no output
+        sage: sage_executable = os.path.join(SAGE_ROOT, 'sage')
+        sage: sage_makedirs(sage_executable)
+        Traceback (most recent call last):
+        ...
+        OSError: ...
     """
     try:
         os.makedirs(dir)
-    except OSError as err:
-        import errno
-        if not err.errno == errno.EEXIST:
+    except OSError:
+        if not os.path.isdir(dir):
             raise
 
 sage_makedirs(SAGE_ROOT)

[jhpalmieri]

Working on sage.math, I edited sage-sage, removing "sage-starts" from the "-tp" section. Then I did

$ export DOT_SAGE=/home/palmieri/.sage-temp/

(note the stupid ending slash, required until someone reviews #11924 -- it's an easy review). Then running

$ sage -tp 16 devel/sage/sage/homology

produced a bunch of errors about creating various subdirectories of $DOT_SAGE. After applying the patch here,

$ rm -rf /home/palmieri/.sage-temp/
$ sage -tp 16 devel/sage/sage/homology

ran repeatedly without any errors.

[jdemeyer]

Patch looks good to me. Do you agree with the reviewer patch? Going to test the whole Sage library now, see if it breaks something.

[jhpalmieri]

Yes, the reviewer patch looks good to me.

[jdemeyer]

All tests and long tests pass, so positive review.

[jdemeyer]

Milestone sage-4.7.3 deleted