Skip to content

Commit

Permalink
Fixed #26691 -- Removed checking for a file's existence before deleting.
Browse files Browse the repository at this point in the history
File operations always raise a ENOENT error when a file doesn't exist.
Checking the file exists before the operation adds a race condition
condition where the file could be removed between operations. As the
operation already raises an error on a missing file, avoid this race and
avoid checking the file exists twice. Instead only check a file exists
by catching the ENOENT error.
  • Loading branch information
jdufresne authored and timgraham committed May 31, 2016
1 parent e3877c5 commit 359be1c
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 23 deletions.
15 changes: 7 additions & 8 deletions django/core/cache/backends/filebased.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,13 @@ def add(self, key, value, timeout=DEFAULT_TIMEOUT, version=None):

def get(self, key, default=None, version=None):
fname = self._key_to_file(key, version)
if os.path.exists(fname):
try:
with io.open(fname, 'rb') as f:
if not self._is_expired(f):
return pickle.loads(zlib.decompress(f.read()))
except IOError as e:
if e.errno == errno.ENOENT:
pass # Cache file was removed after the exists check
try:
with io.open(fname, 'rb') as f:
if not self._is_expired(f):
return pickle.loads(zlib.decompress(f.read()))
except IOError as e:
if e.errno == errno.ENOENT:
pass # Cache file doesn't exist.
return default

def set(self, key, value, timeout=DEFAULT_TIMEOUT, version=None):
Expand Down
16 changes: 7 additions & 9 deletions django/core/files/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -380,15 +380,13 @@ def delete(self, name):
assert name, "The name argument is not allowed to be empty."
name = self.path(name)
# If the file exists, delete it from the filesystem.
# Note that there is a race between os.path.exists and os.remove:
# if os.remove fails with ENOENT, the file was removed
# concurrently, and we can continue normally.
if os.path.exists(name):
try:
os.remove(name)
except OSError as e:
if e.errno != errno.ENOENT:
raise
# If os.remove() fails with ENOENT, the file may have been removed
# concurrently, and it's safe to continue normally.
try:
os.remove(name)
except OSError as e:
if e.errno != errno.ENOENT:
raise

def exists(self, name):
return os.path.exists(self.path(name))
Expand Down
11 changes: 5 additions & 6 deletions tests/staticfiles_tests/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,11 @@ def listdir(self, path):

def delete(self, name):
name = self._path(name)
if os.path.exists(name):
try:
os.remove(name)
except OSError as e:
if e.errno != errno.ENOENT:
raise
try:
os.remove(name)
except OSError as e:
if e.errno != errno.ENOENT:
raise

def path(self, name):
raise NotImplementedError
Expand Down

0 comments on commit 359be1c

Please sign in to comment.