#11972 closed enhancement (fixed)
avoid race conditions when creating directories
Reported by:  jhpalmieri  Owned by:  jason 

Priority:  major  Milestone:  sage4.8 
Component:  misc  Keywords:  
Cc:  Merged in:  sage4.8.alpha0  
Authors:  John Palmieri  Reviewers:  Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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 directorycreating code in Sage to do it the first way.
Apply trac_11972mkdir.patch and 11972_review.patch
Attachments (2)
Change History (14)
comment:1 Changed 8 years ago by
 Status changed from new to needs_review
comment:2 Changed 8 years ago by
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 sagestarts
before running any tests, not just sageptest
(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
 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
 Description modified (diff)
comment:5 Changed 8 years ago by
 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): 55 55 56 56 EXAMPLES:: 57 57 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: ... 59 65 """ 60 66 try: 61 67 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): 65 70 raise 66 71 67 72 sage_makedirs(SAGE_ROOT)
comment:6 Changed 8 years ago by
Working on sage.math, I edited sagesage, removing "sagestarts" from the "tp" section. Then I did
$ export DOT_SAGE=/home/palmieri/.sagetemp/
(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/.sagetemp/ $ sage tp 16 devel/sage/sage/homology
ran repeatedly without any errors.
Changed 8 years ago by
Changed 8 years ago by
comment:7 Changed 8 years ago by
 Description modified (diff)
 Reviewers set to Jeroen Demeyer
comment:8 Changed 8 years ago by
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
Yes, the reviewer patch looks good to me.
comment:10 Changed 8 years ago by
 Merged in set to sage4.7.3.alpha0
 Resolution set to fixed
 Status changed from needs_review to closed
All tests and long tests pass, so positive review.
comment:12 Changed 8 years ago by
 Merged in changed from sage4.7.3.alpha0 to sage4.8.alpha0
 Milestone set to sage4.8
The attached patch should take care of most of the places where Sage creates directories on startup. The exceptions (as far as I can tell) are .sage/ipython and .sage/matplotlibVERSION. 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.