Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix segmentation fault #21436

Merged
merged 3 commits into from May 6, 2022
Merged

BUG: Fix segmentation fault #21436

merged 3 commits into from May 6, 2022

Conversation

JohnnyOu
Copy link
Contributor

@JohnnyOu JohnnyOu commented May 3, 2022

BUG: Fixed segmentation fault with numpy.ufunc.at when ufunc has
nontrivial signature, issue #21301.

Test cases were added in numpy/core/tests/test_ufunc.py to show that
when numpy.ufunc.at uses ufunc with nontrivial signature, then
TypeError should be raised, instead of a potential segmentation fault.
Test cases were also added to show that ufunc with trivial signature is
still valid and works as intended. See issue #21301.

@charris charris changed the title Fixed #21301 BUG: Fix segmentation fault May 3, 2022
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label May 3, 2022
@charris charris added this to the 1.22.4 release milestone May 3, 2022
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me, just two tiny nitpicks.
We could improve the error a tiny bit (although nobody is likely to read it anyway, and IIRC it was actually copied).

@@ -5888,6 +5888,13 @@ ufunc_at(PyUFuncObject *ufunc, PyObject *args)

NPY_BEGIN_THREADS_DEF;

if (ufunc->core_enabled) {
PyErr_Format(PyExc_TypeError,
"numpy.ufunc.at does not support ufunc with non-trivial "\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit, but the \ is not necessary :).

We could probably make the error message friendlier by printing out the ufunc name rather than "numpy.ufunc.at". The name is far more interesting than the actual core signature!

a = np.array([[[1, 2], [3, 4]]])
assert_raises(TypeError, np.linalg._umath_linalg.det.at, a, [0])

# Ufunc with None signature should work as intended
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should have tests for this already? So I think you can remove them again.
If you think they add something, just move them above to that long list for now...

assert_raises(TypeError, np.matmul.at, a, [0], b)

a = np.array([[[1, 2], [3, 4]]])
assert_raises(TypeError, np.linalg._umath_linalg.det.at, a, [0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests look good. But single test functions getting too long tends to be inconvenient. Could you make a new method for these two? Could just be something mentioning that gufuncs don't support at or so...

@JohnnyOu
Copy link
Contributor Author

JohnnyOu commented May 4, 2022

I have modified the functions as requested! The error message is improved, displaying both the ufunc's name and signature, and the test cases (checking that the ufunc.at throws error on nontrivial signatures) were moved to their own method.

@JohnnyOu
Copy link
Contributor Author

JohnnyOu commented May 4, 2022

When can I expect this to get merged since the issue is fixed?

@charris
Copy link
Member

charris commented May 5, 2022

@seberg Does this look good to you?

@seberg
Copy link
Member

seberg commented May 5, 2022

Just a tiny fix, but just did that directly with a comment. Otherwise, I think it is good to go in, though.

@seberg
Copy link
Member

seberg commented May 6, 2022

Thanks @JohnnyOu!

@seberg seberg merged commit ba54f56 into numpy:main May 6, 2022
@InessaPawson
Copy link
Member

Hi-five on merging your first pull request to NumPy, @JohnnyOu! We hope you stick around! Your choices aren’t limited to programming – you can review pull requests, help us stay on top of new and old issues, develop educational material, work on our website, add or improve graphic design, create marketing materials, translate website content, write grant proposals, and help with other fundraising initiatives. For more info, check out: https://numpy.org/contribute
Also, consider joining our mailing list. This is a great way to connect with other cool people in our community and be part of important conversations that affect the development of NumPy: https://mail.python.org/mailman/listinfo/numpy-discussion

charris pushed a commit to charris/numpy that referenced this pull request May 8, 2022
* Fixed numpy#21301

* Fixed numpy#21436, better error message, organized test cases as requested

* Update numpy/core/src/umath/ufunc_object.c

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label May 8, 2022
@charris charris removed this from the 1.22.4 release milestone May 8, 2022
charris added a commit that referenced this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants