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: Segfault on np.random.set_state(()) #25402

Closed
Jasha10 opened this issue Dec 16, 2023 · 5 comments · Fixed by #25426
Closed

BUG: Segfault on np.random.set_state(()) #25402

Jasha10 opened this issue Dec 16, 2023 · 5 comments · Fixed by #25426
Labels
00 - Bug 09 - Backport-Candidate PRs tagged should be backported

Comments

@Jasha10
Copy link

Jasha10 commented Dec 16, 2023

Describe the issue:

When I call np.random.set_state(()) (passing in a tuple (), which is not an expected argument) I get a segfault

Reproduce the code example:

$ python
Python 3.10.13 | packaged by conda-forge | (main, Oct 26 2023, 18:07:37) [GCC 12.3.0] on linux
>>> import numpy as np
>>> np.random.set_state(())
Segmentation fault (core dumped)

Python and NumPy Versions:

>>> import sys, numpy; print(numpy.__version__); print(sys.version)
1.22.4
3.10.13 | packaged by conda-forge | (main, Oct 26 2023, 18:07:37) [GCC 12.3.0]
@ngoldbaum
Copy link
Member

I can reproduce this. It looks like it's segfaulting inside of tuple getitem when it tries to access state[0] here. Since it's an empty tuple that item doesn't exist.

I'm a little surprised cython doesn't raise a ValueError instead.

A quick fix would be to check for a sequence of the correct length with the types we expect.

That said, this is in the legacy RNG interface. I'm not sure if we're still updating it. But we should probably avoid seg faults in the python API, even if it is a legacy API.

@ngoldbaum
Copy link
Member

Ah, another thing we could do is wrap just that one line in a with cython.boundscheck(True). I see at the top of the file bounds checking is disabled. This is one reason it's a really bad idea to do that.

@rkern would a one-line change to mtrand.pyx to avoid a seg fault when someone hands it invalid input be OK?

@eendebakpt
Copy link
Contributor

eendebakpt commented Dec 17, 2023

@ngoldbaum The problem is caused by the second line in mtrand.pyx:

#!python
#cython: wraparound=False, nonecheck=False, boundscheck=False, cdivision=True, language_level=3, binding=True
import operator
...

If compiled with boundscheck=True an IndexError is generated.

@ngoldbaum
Copy link
Member

Right, but this is the legacy RNG interface which we're not supposed to make major changes in. Turning on bounds checking for the whole file might be a serious performance degradation for some workflows. Turning it on for one line in the input validation is less likely to impact users.

@rkern
Copy link
Member

rkern commented Dec 17, 2023

@rkern would a one-line change to mtrand.pyx to avoid a seg fault when someone hands it invalid input be OK?

Yes, preventing segfaults is in-bounds (no pun intended, but appreciated after the fact).

I do prefer the one-line change rather than the whole-file change for the reasons you give.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
00 - Bug 09 - Backport-Candidate PRs tagged should be backported
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants