Opened 10 years ago

Closed 10 years ago

Last modified 10 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:

Status badges

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 10 years ago.
11972_review.patch (2.3 KB) - added by jdemeyer 10 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 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 10 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 10 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 10 years ago by jdemeyer

  • Description modified (diff)

comment:5 Changed 10 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 10 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 10 years ago by jhpalmieri

Changed 10 years ago by jdemeyer

comment:7 Changed 10 years ago by jdemeyer

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

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

Yes, the reviewer patch looks good to me.

comment:10 Changed 10 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 10 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:12 Changed 10 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.