Skip to content

Commit 854b5b9

Browse files
committed
Revise so close() doesn't wait for mutex.
This makes it not safe to share iterators between multiple threads so document that.
1 parent 00267a2 commit 854b5b9

4 files changed

Lines changed: 85 additions & 38 deletions

File tree

Doc/library/os.rst

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2956,6 +2956,11 @@ features:
29562956

29572957
.. audit-event:: os.scandir path os.scandir
29582958

2959+
Sharing a :func:`scandir` iterator between threads will not corrupt the
2960+
iterator, but it is subject to :term:`race conditions <race condition>`:
2961+
which entries each thread receives is unspecified, and closing the iterator
2962+
while another thread is iterating ends that iteration early.
2963+
29592964
The :func:`scandir` iterator supports the :term:`context manager` protocol
29602965
and has the following method:
29612966

Lib/test/test_os/test_os.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5352,6 +5352,18 @@ def test_resource_warning(self):
53525352
with self.check_no_resource_warning():
53535353
del iterator
53545354

5355+
def test_no_resource_warning_when_open_fails(self):
5356+
# gh-152754: a scandir() call that never opened a directory owns
5357+
# nothing, and must not report an unclosed iterator.
5358+
self.create_file("file.txt")
5359+
missing = os.path.join(self.path, "missing")
5360+
not_a_dir = os.path.join(self.path, "file.txt")
5361+
for path in (missing, not_a_dir):
5362+
with self.subTest(path=path):
5363+
with self.check_no_resource_warning():
5364+
with self.assertRaises(OSError):
5365+
os.scandir(path)
5366+
53555367

53565368
@threading_helper.requires_working_threading()
53575369
class ScandirThreadingTest(unittest.TestCase):
Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
Fix a crash when the same :func:`os.scandir` iterator is used concurrently
2-
from multiple threads. Iterating and closing the iterator are now serialized
3-
with a per-iterator lock, so that closing it no longer releases the
4-
directory handle while another thread is still reading from it.
2+
from multiple threads. It no longer releases the directory handle while
3+
another thread is reading from it. Sharing an iterator between threads
4+
remains subject to race conditions: which entries each thread receives is
5+
unspecified.

Modules/posixmodule.c

Lines changed: 64 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@
2121
#include "pycore_import.h" // _PyImport_AcquireLock()
2222
#include "pycore_initconfig.h" // _PyStatus_EXCEPTION()
2323
#include "pycore_jit_unwind.h" // _Py_jit_debug_mutex
24+
#include "pycore_lock.h" // _PyMutex_LockTimed()
2425
#include "pycore_long.h" // _PyLong_IsNegative()
2526
#include "pycore_moduleobject.h" // _PyModule_GetState()
2627
#include "pycore_object.h" // _PyObject_LookupSpecial()
27-
#include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_INT_RELAXED()
2828
#include "pycore_pylifecycle.h" // _PyOS_URandom()
2929
#include "pycore_pystate.h" // _PyInterpreterState_GET()
3030
#include "pycore_signal.h" // Py_NSIG
@@ -16952,9 +16952,13 @@ typedef struct {
1695216952
#ifdef HAVE_FDOPENDIR
1695316953
int fd;
1695416954
#endif
16955-
// Protects the iterator state when an os.scandir() iterator is used from
16956-
// multiple threads.
16957-
PyMutex mutex;
16955+
// Sharing the iterator between threads is subject to race conditions:
16956+
// which entries each thread receives is unspecified. It must not
16957+
// corrupt the iterator or crash. Since we don't want close() to be
16958+
// held up by a blocking directory read, we set the 'closed' flag if
16959+
// there are reads in progress.
16960+
PyMutex read_mutex;
16961+
uint8_t closed;
1695816962
} ScandirIterator;
1695916963

1696016964
#define ScandirIterator_CAST(op) ((ScandirIterator *)(op))
@@ -16964,19 +16968,21 @@ typedef struct {
1696416968
static int
1696516969
ScandirIterator_is_closed(ScandirIterator *iterator)
1696616970
{
16967-
PyMutex_Lock(&iterator->mutex);
16968-
int closed = iterator->handle == INVALID_HANDLE_VALUE;
16969-
PyMutex_Unlock(&iterator->mutex);
16970-
return closed;
16971+
return _Py_atomic_load_uint8(&iterator->closed);
1697116972
}
1697216973

1697316974
static void
1697416975
ScandirIterator_closedir(ScandirIterator *iterator)
1697516976
{
16976-
PyMutex_Lock(&iterator->mutex);
16977-
HANDLE handle = iterator->handle;
16978-
iterator->handle = INVALID_HANDLE_VALUE;
16979-
PyMutex_Unlock(&iterator->mutex);
16977+
HANDLE handle = INVALID_HANDLE_VALUE;
16978+
16979+
_Py_atomic_store_uint8(&iterator->closed, 1);
16980+
if (_PyMutex_LockTimed(&iterator->read_mutex, 0, 0) == PY_LOCK_ACQUIRED) {
16981+
// no reads in progress, we can close the handle
16982+
handle = iterator->handle;
16983+
iterator->handle = INVALID_HANDLE_VALUE;
16984+
PyMutex_Unlock(&iterator->read_mutex);
16985+
}
1698016986

1698116987
if (handle != INVALID_HANDLE_VALUE) {
1698216988
Py_BEGIN_ALLOW_THREADS
@@ -16994,9 +17000,11 @@ ScandirIterator_iternext(PyObject *op)
1699417000
DWORD error = ERROR_SUCCESS;
1699517001
int found = 0;
1699617002

16997-
PyMutex_Lock(&iterator->mutex);
17003+
PyMutex_Lock(&iterator->read_mutex);
1699817004
/* Happens if the iterator is iterated twice, or closed explicitly */
16999-
while (iterator->handle != INVALID_HANDLE_VALUE) {
17005+
while (iterator->handle != INVALID_HANDLE_VALUE &&
17006+
!_Py_atomic_load_uint8_relaxed(&iterator->closed))
17007+
{
1700017008
if (!iterator->first_time) {
1700117009
Py_BEGIN_ALLOW_THREADS
1700217010
success = FindNextFileW(iterator->handle, &iterator->file_data);
@@ -17021,7 +17029,11 @@ ScandirIterator_iternext(PyObject *op)
1702117029

1702217030
/* Loop till we get a non-dot directory or finish iterating */
1702317031
}
17024-
PyMutex_Unlock(&iterator->mutex);
17032+
PyMutex_Unlock(&iterator->read_mutex);
17033+
17034+
if (found && ScandirIterator_is_closed(iterator)) {
17035+
ScandirIterator_closedir(iterator); // deferred close
17036+
}
1702517037

1702617038
if (found) {
1702717039
PyObject *module = PyType_GetModule(Py_TYPE(iterator));
@@ -17045,19 +17057,21 @@ ScandirIterator_iternext(PyObject *op)
1704517057
static int
1704617058
ScandirIterator_is_closed(ScandirIterator *iterator)
1704717059
{
17048-
PyMutex_Lock(&iterator->mutex);
17049-
int closed = iterator->dirp == NULL;
17050-
PyMutex_Unlock(&iterator->mutex);
17051-
return closed;
17060+
return _Py_atomic_load_uint8(&iterator->closed);
1705217061
}
1705317062

1705417063
static void
1705517064
ScandirIterator_closedir(ScandirIterator *iterator)
1705617065
{
17057-
PyMutex_Lock(&iterator->mutex);
17058-
DIR *dirp = iterator->dirp;
17059-
iterator->dirp = NULL;
17060-
PyMutex_Unlock(&iterator->mutex);
17066+
DIR *dirp = NULL;
17067+
17068+
_Py_atomic_store_uint8(&iterator->closed, 1);
17069+
if (_PyMutex_LockTimed(&iterator->read_mutex, 0, 0) == PY_LOCK_ACQUIRED) {
17070+
// no reads in progress, we can close dirp
17071+
dirp = iterator->dirp;
17072+
iterator->dirp = NULL;
17073+
PyMutex_Unlock(&iterator->read_mutex);
17074+
}
1706117075

1706217076
if (dirp != NULL) {
1706317077
Py_BEGIN_ALLOW_THREADS
@@ -17081,15 +17095,18 @@ ScandirIterator_iternext(PyObject *op)
1708117095
int found = 0;
1708217096
int error = 0;
1708317097
int no_memory = 0;
17084-
char *name = NULL;
17098+
char namebuf[256];
17099+
char *name = namebuf;
1708517100
ino_t d_ino = 0;
1708617101
#ifdef HAVE_DIRENT_D_TYPE
1708717102
unsigned char d_type = 0;
1708817103
#endif
1708917104

17090-
PyMutex_Lock(&iterator->mutex);
17105+
PyMutex_Lock(&iterator->read_mutex);
1709117106
/* Happens if the iterator is iterated twice, or closed explicitly */
17092-
while (iterator->dirp != NULL) {
17107+
while (iterator->dirp != NULL &&
17108+
!_Py_atomic_load_uint8_relaxed(&iterator->closed))
17109+
{
1709317110
Py_BEGIN_ALLOW_THREADS
1709417111
errno = 0;
1709517112
direntp = readdir(iterator->dirp);
@@ -17107,10 +17124,12 @@ ScandirIterator_iternext(PyObject *op)
1710717124
is_dot = direntp->d_name[0] == '.' &&
1710817125
(name_len == 1 || (direntp->d_name[1] == '.' && name_len == 2));
1710917126
if (!is_dot) {
17110-
name = PyMem_RawMalloc(name_len + 1);
17111-
if (name == NULL) {
17112-
no_memory = 1;
17113-
break;
17127+
if ((size_t)name_len >= sizeof(namebuf)) {
17128+
name = PyMem_RawMalloc(name_len + 1);
17129+
if (name == NULL) {
17130+
no_memory = 1;
17131+
break;
17132+
}
1711417133
}
1711517134
memcpy(name, direntp->d_name, name_len);
1711617135
name[name_len] = '\0';
@@ -17124,7 +17143,11 @@ ScandirIterator_iternext(PyObject *op)
1712417143

1712517144
/* Loop till we get a non-dot directory or finish iterating */
1712617145
}
17127-
PyMutex_Unlock(&iterator->mutex);
17146+
PyMutex_Unlock(&iterator->read_mutex);
17147+
17148+
if (found && ScandirIterator_is_closed(iterator)) {
17149+
ScandirIterator_closedir(iterator); // deferred close
17150+
}
1712817151

1712917152
if (found) {
1713017153
PyObject *module = PyType_GetModule(Py_TYPE(iterator));
@@ -17135,7 +17158,9 @@ ScandirIterator_iternext(PyObject *op)
1713517158
, d_type
1713617159
#endif
1713717160
);
17138-
PyMem_RawFree(name);
17161+
if (name != namebuf) {
17162+
PyMem_RawFree(name);
17163+
}
1713917164
if (entry != NULL) {
1714017165
return entry;
1714117166
}
@@ -17184,9 +17209,11 @@ ScandirIterator_finalize(PyObject *op)
1718417209
/* Save the current exception, if any. */
1718517210
PyObject *exc = PyErr_GetRaisedException();
1718617211

17187-
if (!ScandirIterator_is_closed(iterator)) {
17188-
ScandirIterator_closedir(iterator);
17212+
int was_closed = ScandirIterator_is_closed(iterator);
17213+
17214+
ScandirIterator_closedir(iterator);
1718917215

17216+
if (!was_closed) {
1719017217
if (PyErr_ResourceWarning(op, 1,
1719117218
"unclosed scandir iterator %R", iterator))
1719217219
{
@@ -17284,7 +17311,8 @@ os_scandir_impl(PyObject *module, path_t *path)
1728417311
if (!iterator)
1728517312
return NULL;
1728617313

17287-
iterator->mutex = (PyMutex){0};
17314+
iterator->read_mutex = (PyMutex){0};
17315+
iterator->closed = 1;
1728817316
#ifdef MS_WINDOWS
1728917317
iterator->handle = INVALID_HANDLE_VALUE;
1729017318
#else
@@ -17357,6 +17385,7 @@ os_scandir_impl(PyObject *module, path_t *path)
1735717385
}
1735817386
#endif
1735917387

17388+
iterator->closed = 0;
1736017389
return (PyObject *)iterator;
1736117390

1736217391
error:

0 commit comments

Comments
 (0)