Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#11972 closed enhancement (fixed)

avoid race conditions when creating directories

Reported by: jhpalmieri Owned by: jason
Priority: major Milestone: sage-4.8
Component: misc Keywords:
Cc: Merged in: sage-4.8.alpha0
Authors: John Palmieri Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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

Attachments (2)

trac_11972-mkdir.patch (9.5 KB) - added by jhpalmieri 8 years ago.
11972_review.patch (2.3 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 8 years ago by jhpalmieri

  • Status changed from new to needs_review

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.

comment:2 Changed 8 years ago by 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).

comment:3 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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...

comment:4 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 8 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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

  • sage/misc/misc.py

    diff --git a/sage/misc/misc.py b/sage/misc/misc.py
    a b def sage_makedirs(dir): 
    5555
    5656    EXAMPLES::
    5757
    58         sage: sage_makedirs(DOT_SAGE)
     58        sage: from sage.misc.misc import sage_makedirs
     59        sage: sage_makedirs(DOT_SAGE) # no output
     60        sage: sage_executable = os.path.join(SAGE_ROOT, 'sage')
     61        sage: sage_makedirs(sage_executable)
     62        Traceback (most recent call last):
     63        ...
     64        OSError: ...
    5965    """
    6066    try:
    6167        os.makedirs(dir)
    62     except OSError as err:
    63         import errno
    64         if not err.errno == errno.EEXIST:
     68    except OSError:
     69        if not os.path.isdir(dir):
    6570            raise
    6671
    6772sage_makedirs(SAGE_ROOT)

comment:6 Changed 8 years ago by 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.

Changed 8 years ago by jhpalmieri

Changed 8 years ago by jdemeyer

comment:7 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers set to Jeroen Demeyer

comment:8 Changed 8 years ago by 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.

comment:9 Changed 8 years ago by jhpalmieri

Yes, the reviewer patch looks good to me.

comment:10 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.3.alpha0
  • Resolution set to fixed
  • Status changed from needs_review to closed

All tests and long tests pass, so positive review.

comment:11 Changed 8 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:12 Changed 8 years ago by jdemeyer

  • Merged in changed from sage-4.7.3.alpha0 to sage-4.8.alpha0
  • Milestone set to sage-4.8
Note: See TracTickets for help on using tickets.