Skip to content

Commit 56f8d5b

Browse files
charrischarris
authored
Merge pull request #28321 from charris/backport-28290
BUG: fix race initializing legacy dtype casts
2 parents 34e233e + 48515a3 commit 56f8d5b

File tree

7 files changed

+438
-72
lines changed

7 files changed

+438
-72
lines changed

‎.github/workflows/compiler_sanitizers.yml‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
name: Test with compiler sanitizers
2+
3+
on:
4+
push:
5+
branches:
6+
- main
7+
pull_request:
8+
branches:
9+
- main
10+
- maintenance/**
11+
12+
defaults:
13+
run:
14+
shell: bash
15+
16+
concurrency:
17+
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
18+
cancel-in-progress: true
19+
20+
permissions:
21+
contents: read # to fetch code (actions/checkout)
22+
23+
jobs:
24+
clang_ASAN:
25+
# To enable this workflow on a fork, comment out:
26+
if: github.repository == 'numpy/numpy'
27+
runs-on: macos-latest
28+
steps:
29+
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
30+
with:
31+
submodules: recursive
32+
fetch-tags: true
33+
persist-credentials: false
34+
- name: Set up pyenv
35+
run: |
36+
git clone https://github.com/pyenv/pyenv.git "$HOME/.pyenv"
37+
PYENV_ROOT="$HOME/.pyenv"
38+
PYENV_BIN="$PYENV_ROOT/bin"
39+
PYENV_SHIMS="$PYENV_ROOT/shims"
40+
echo "$PYENV_BIN" >> $GITHUB_PATH
41+
echo "$PYENV_SHIMS" >> $GITHUB_PATH
42+
echo "PYENV_ROOT=$PYENV_ROOT" >> $GITHUB_ENV
43+
- name: Check pyenv is working
44+
run:
45+
pyenv --version
46+
- name: Set up LLVM
47+
run: |
48+
brew install llvm@19
49+
LLVM_PREFIX=$(brew --prefix llvm@19)
50+
echo CC="$LLVM_PREFIX/bin/clang" >> $GITHUB_ENV
51+
echo CXX="$LLVM_PREFIX/bin/clang++" >> $GITHUB_ENV
52+
echo LDFLAGS="-L$LLVM_PREFIX/lib" >> $GITHUB_ENV
53+
echo CPPFLAGS="-I$LLVM_PREFIX/include" >> $GITHUB_ENV
54+
- name: Build Python with address sanitizer
55+
run: |
56+
CONFIGURE_OPTS="--with-address-sanitizer" pyenv install 3.13
57+
pyenv global 3.13
58+
- name: Install dependencies
59+
run: |
60+
pip install -r requirements/build_requirements.txt
61+
pip install -r requirements/ci_requirements.txt
62+
pip install -r requirements/test_requirements.txt
63+
# xdist captures stdout/stderr, but we want the ASAN output
64+
pip uninstall -y pytest-xdist
65+
- name: Build
66+
run:
67+
python -m spin build -j2 -- -Db_sanitize=address
68+
- name: Test
69+
run: |
70+
# pass -s to pytest to see ASAN errors and warnings, otherwise pytest captures them
71+
ASAN_OPTIONS=detect_leaks=0:symbolize=1:strict_init_order=true:allocator_may_return_null=1:halt_on_error=1 \
72+
python -m spin test -- -v -s --timeout=600 --durations=10
73+
74+
clang_TSAN:
75+
# To enable this workflow on a fork, comment out:
76+
if: github.repository == 'numpy/numpy'
77+
runs-on: macos-latest
78+
steps:
79+
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
80+
with:
81+
submodules: recursive
82+
fetch-tags: true
83+
persist-credentials: false
84+
- name: Set up pyenv
85+
run: |
86+
git clone https://github.com/pyenv/pyenv.git "$HOME/.pyenv"
87+
PYENV_ROOT="$HOME/.pyenv"
88+
PYENV_BIN="$PYENV_ROOT/bin"
89+
PYENV_SHIMS="$PYENV_ROOT/shims"
90+
echo "$PYENV_BIN" >> $GITHUB_PATH
91+
echo "$PYENV_SHIMS" >> $GITHUB_PATH
92+
echo "PYENV_ROOT=$PYENV_ROOT" >> $GITHUB_ENV
93+
- name: Check pyenv is working
94+
run:
95+
pyenv --version
96+
- name: Set up LLVM
97+
run: |
98+
brew install llvm@19
99+
LLVM_PREFIX=$(brew --prefix llvm@19)
100+
echo CC="$LLVM_PREFIX/bin/clang" >> $GITHUB_ENV
101+
echo CXX="$LLVM_PREFIX/bin/clang++" >> $GITHUB_ENV
102+
echo LDFLAGS="-L$LLVM_PREFIX/lib" >> $GITHUB_ENV
103+
echo CPPFLAGS="-I$LLVM_PREFIX/include" >> $GITHUB_ENV
104+
- name: Build Python with thread sanitizer support
105+
run: |
106+
# free-threaded Python is much more likely to trigger races
107+
CONFIGURE_OPTS="--with-thread-sanitizer" pyenv install 3.13t
108+
pyenv global 3.13t
109+
- name: Install dependencies
110+
run: |
111+
# TODO: remove when a released cython supports free-threaded python
112+
pip install -i https://pypi.anaconda.org/scientific-python-nightly-wheels/simple cython
113+
pip install -r requirements/build_requirements.txt
114+
pip install -r requirements/ci_requirements.txt
115+
pip install -r requirements/test_requirements.txt
116+
# xdist captures stdout/stderr, but we want the TSAN output
117+
pip uninstall -y pytest-xdist
118+
- name: Build
119+
run:
120+
python -m spin build -j2 -- -Db_sanitize=thread
121+
- name: Test
122+
run: |
123+
# These tests are slow, so only run tests in files that do "import threading" to make them count
124+
TSAN_OPTIONS=allocator_may_return_null=1:halt_on_error=1 \
125+
python -m spin test \
126+
`find numpy -name "test*.py" | xargs grep -l "import threading" | tr '\n' ' '` \
127+
-- -v -s --timeout=600 --durations=10

‎numpy/_core/src/multiarray/convert_datatype.c‎

Lines changed: 93 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -62,46 +62,24 @@ static PyObject *
6262
PyArray_GetObjectToGenericCastingImpl(void);
6363

6464

65-
/**
66-
* Fetch the casting implementation from one DType to another.
67-
*
68-
* @param from The implementation to cast from
69-
* @param to The implementation to cast to
70-
*
71-
* @returns A castingimpl (PyArrayDTypeMethod *), None or NULL with an
72-
* error set.
73-
*/
74-
NPY_NO_EXPORT PyObject *
75-
PyArray_GetCastingImpl(PyArray_DTypeMeta *from, PyArray_DTypeMeta *to)
65+
static PyObject *
66+
create_casting_impl(PyArray_DTypeMeta *from, PyArray_DTypeMeta *to)
7667
{
77-
PyObject *res;
78-
if (from == to) {
79-
res = (PyObject *)NPY_DT_SLOTS(from)->within_dtype_castingimpl;
80-
}
81-
else {
82-
res = PyDict_GetItemWithError(NPY_DT_SLOTS(from)->castingimpls, (PyObject *)to);
83-
}
84-
if (res != NULL || PyErr_Occurred()) {
85-
Py_XINCREF(res);
86-
return res;
87-
}
8868
/*
89-
* The following code looks up CastingImpl based on the fact that anything
69+
* Look up CastingImpl based on the fact that anything
9070
* can be cast to and from objects or structured (void) dtypes.
91-
*
92-
* The last part adds casts dynamically based on legacy definition
9371
*/
9472
if (from->type_num == NPY_OBJECT) {
95-
res = PyArray_GetObjectToGenericCastingImpl();
73+
return PyArray_GetObjectToGenericCastingImpl();
9674
}
9775
else if (to->type_num == NPY_OBJECT) {
98-
res = PyArray_GetGenericToObjectCastingImpl();
76+
return PyArray_GetGenericToObjectCastingImpl();
9977
}
10078
else if (from->type_num == NPY_VOID) {
101-
res = PyArray_GetVoidToGenericCastingImpl();
79+
return PyArray_GetVoidToGenericCastingImpl();
10280
}
10381
else if (to->type_num == NPY_VOID) {
104-
res = PyArray_GetGenericToVoidCastingImpl();
82+
return PyArray_GetGenericToVoidCastingImpl();
10583
}
10684
/*
10785
* Reject non-legacy dtypes. They need to use the new API to add casts and
@@ -125,42 +103,105 @@ PyArray_GetCastingImpl(PyArray_DTypeMeta *from, PyArray_DTypeMeta *to)
125103
from->singleton, to->type_num);
126104
if (castfunc == NULL) {
127105
PyErr_Clear();
128-
/* Remember that this cast is not possible */
129-
if (PyDict_SetItem(NPY_DT_SLOTS(from)->castingimpls,
130-
(PyObject *) to, Py_None) < 0) {
131-
return NULL;
132-
}
133106
Py_RETURN_NONE;
134107
}
135108
}
136-
137-
/* PyArray_AddLegacyWrapping_CastingImpl find the correct casting level: */
138-
/*
139-
* TODO: Possibly move this to the cast registration time. But if we do
140-
* that, we have to also update the cast when the casting safety
141-
* is registered.
109+
/* Create a cast using the state of the legacy casting setup defined
110+
* during the setup of the DType.
111+
*
112+
* Ideally we would do this when we create the DType, but legacy user
113+
* DTypes don't have a way to signal that a DType is done setting up
114+
* casts. Without such a mechanism, the safest way to know that a
115+
* DType is done setting up is to register the cast lazily the first
116+
* time a user does the cast.
117+
*
118+
* We *could* register the casts when we create the wrapping
119+
* DTypeMeta, but that means the internals of the legacy user DType
120+
* system would need to update the state of the casting safety flags
121+
* in the cast implementations stored on the DTypeMeta. That's an
122+
* inversion of abstractions and would be tricky to do without
123+
* creating circular dependencies inside NumPy.
142124
*/
143125
if (PyArray_AddLegacyWrapping_CastingImpl(from, to, -1) < 0) {
144126
return NULL;
145127
}
128+
/* castingimpls is unconditionally filled by
129+
* AddLegacyWrapping_CastingImpl, so this won't create a recursive
130+
* critical section
131+
*/
146132
return PyArray_GetCastingImpl(from, to);
147133
}
134+
}
148135

149-
if (res == NULL) {
136+
static PyObject *
137+
ensure_castingimpl_exists(PyArray_DTypeMeta *from, PyArray_DTypeMeta *to)
138+
{
139+
int return_error = 0;
140+
PyObject *res = NULL;
141+
142+
/* Need to create the cast. This might happen at runtime so we enter a
143+
critical section to avoid races */
144+
145+
Py_BEGIN_CRITICAL_SECTION(NPY_DT_SLOTS(from)->castingimpls);
146+
147+
/* check if another thread filled it while this thread was blocked on
148+
acquiring the critical section */
149+
if (PyDict_GetItemRef(NPY_DT_SLOTS(from)->castingimpls, (PyObject *)to,
150+
&res) < 0) {
151+
return_error = 1;
152+
}
153+
else if (res == NULL) {
154+
res = create_casting_impl(from, to);
155+
if (res == NULL) {
156+
return_error = 1;
157+
}
158+
else if (PyDict_SetItem(NPY_DT_SLOTS(from)->castingimpls,
159+
(PyObject *)to, res) < 0) {
160+
return_error = 1;
161+
}
162+
}
163+
Py_END_CRITICAL_SECTION();
164+
if (return_error) {
165+
Py_XDECREF(res);
150166
return NULL;
151167
}
152-
if (from == to) {
168+
if (from == to && res == Py_None) {
153169
PyErr_Format(PyExc_RuntimeError,
154170
"Internal NumPy error, within-DType cast missing for %S!", from);
155171
Py_DECREF(res);
156172
return NULL;
157173
}
158-
if (PyDict_SetItem(NPY_DT_SLOTS(from)->castingimpls,
159-
(PyObject *)to, res) < 0) {
160-
Py_DECREF(res);
174+
return res;
175+
}
176+
177+
/**
178+
* Fetch the casting implementation from one DType to another.
179+
*
180+
* @param from The implementation to cast from
181+
* @param to The implementation to cast to
182+
*
183+
* @returns A castingimpl (PyArrayDTypeMethod *), None or NULL with an
184+
* error set.
185+
*/
186+
NPY_NO_EXPORT PyObject *
187+
PyArray_GetCastingImpl(PyArray_DTypeMeta *from, PyArray_DTypeMeta *to)
188+
{
189+
PyObject *res = NULL;
190+
if (from == to) {
191+
if ((NPY_DT_SLOTS(from)->within_dtype_castingimpl) != NULL) {
192+
res = Py_XNewRef(
193+
(PyObject *)NPY_DT_SLOTS(from)->within_dtype_castingimpl);
194+
}
195+
}
196+
else if (PyDict_GetItemRef(NPY_DT_SLOTS(from)->castingimpls,
197+
(PyObject *)to, &res) < 0) {
161198
return NULL;
162199
}
163-
return res;
200+
if (res != NULL) {
201+
return res;
202+
}
203+
204+
return ensure_castingimpl_exists(from, to);
164205
}
165206

166207

@@ -409,7 +450,7 @@ _get_cast_safety_from_castingimpl(PyArrayMethodObject *castingimpl,
409450
* implementations fully to have them available for doing the actual cast
410451
* later.
411452
*
412-
* @param from The descriptor to cast from
453+
* @param from The descriptor to cast from
413454
* @param to The descriptor to cast to (may be NULL)
414455
* @param to_dtype If `to` is NULL, must pass the to_dtype (otherwise this
415456
* is ignored).
@@ -2031,6 +2072,11 @@ PyArray_AddCastingImplementation(PyBoundArrayMethodObject *meth)
20312072
/**
20322073
* Add a new casting implementation using a PyArrayMethod_Spec.
20332074
*
2075+
* Using this function outside of module initialization without holding a
2076+
* critical section on the castingimpls dict may lead to a race to fill the
2077+
* dict. Use PyArray_GetGastingImpl to lazily register casts at runtime
2078+
* safely.
2079+
*
20342080
* @param spec The specification to use as a source
20352081
* @param private If private, allow slots not publicly exposed.
20362082
* @return 0 on success -1 on failure

‎numpy/_core/src/multiarray/dtypemeta.c‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1252,6 +1252,12 @@ dtypemeta_wrap_legacy_descriptor(
12521252
return -1;
12531253
}
12541254
}
1255+
else {
1256+
// ensure the within dtype cast is populated for legacy user dtypes
1257+
if (PyArray_GetCastingImpl(dtype_class, dtype_class) == NULL) {
1258+
return -1;
1259+
}
1260+
}
12551261

12561262
return 0;
12571263
}

0 commit comments

Comments
 (0)