Skip to content

BUG: sanity check __array_interface__ number of dimensions #28407

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 6, 2025
Merged

BUG: sanity check __array_interface__ number of dimensions #28407

merged 1 commit into from
Mar 6, 2025

Conversation

lvllvl
Copy link
Contributor

@lvllvl lvllvl commented Feb 28, 2025 •
edited
Loading

Bug fix to #27949.

  • Added a change to numpy/_core/src/multiarray/ctors.c to avoid segmentation fault
  • added a test to numpy/_core/tests/test_multiarray.py. If the test passes there's no longer a segmentation fault

@lvllvl lvllvl force-pushed the issue-27949 branch 4 times, most recently from c5ab096 to ad293f5 Compare March 1, 2025 02:10
@lvllvl lvllvl changed the title Issue 27949 BUG: numpy.asarray's boundary check fails with too large shape and got a segmentation fault Mar 1, 2025
@lvllvl lvllvl marked this pull request as ready for review March 1, 2025 03:44
@lvllvl lvllvl mentioned this pull request Mar 1, 2025
Closed
seberg
seberg reviewed Mar 2, 2025
View reviewed changes
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.

Looks good, thanks. If we touch this, I think we might as well also fix the bug for ridiculously large tuples here (don't care for a test, since that would require a lot of memory to run).

@lvllvl
Copy link
Contributor Author

lvllvl commented Mar 3, 2025

Looks good, thanks. If we touch this, I think we might as well also fix the bug for ridiculously large tuples here (don't care for a test, since that would require a lot of memory to run).

I wasn't able to find an open issue related to large tuples. Is this a known problem or should we open an issue for it?
Would you like this issue fixed within this PR or fixed in a separate PR?

@lvllvl lvllvl force-pushed the issue-27949 branch 2 times, most recently from f81eb97 to e4c401b Compare March 3, 2025 02:28
@lvllvl lvllvl marked this pull request as draft March 3, 2025 03:31
@seberg
Copy link
Member

seberg commented Mar 3, 2025

Universal test failures. No there is no issue, just something to spot, int is the wrong integer type for PyTuple_SIZE, since it is basically the same code (and it is not like the diff is currently big), I would be happier to just include it in this PR, but works either way.

@lvllvl lvllvl marked this pull request as ready for review March 4, 2025 02:29
@lvllvl
Copy link
Contributor Author

lvllvl commented Mar 4, 2025 •
edited
Loading

Universal test failures. No there is no issue, just something to spot, int is the wrong integer type for PyTuple_SIZE, since it is basically the same code (and it is not like the diff is currently big), I would be happier to just include it in this PR, but works either way.

@seberg ok I think I can make the change for the extremely large tuple that takes up memory. Is there a specific memory limit you had in mind? I can poke around this file, maybe there's a clue in here.

I think I fixed the test failures. n was already a variable in the function and it was of type int. So I changed my variable name and then reassigned it back to n after the fact so tests would build/pass.

seberg
seberg reviewed Mar 5, 2025
View reviewed changes
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.

First is a nit, but I don't like that new overflow check without reason much.
(I am not even sure it can be hit, but maybe it is possible with exact choice of itemsize and prod(shape))

numpy/_core/src/multiarray/ctors.c Outdated
PyErr_SetString(PyExc_ValueError,
"array is too big; overflow in _array_fill_strides.");
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why? Can we please not do this? I am happy to check ridiculous tuple lengths one could claim them to be technically valid also since it is local to the __array_interface__.
But this just can't be valid. Someone might as well pass a bad data pointer if they run into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seberg
Would it be ok to focus on the original bug fix in this PR and then I'll address the large tuple issue separately?

I wasn't able to reproduce the large tuple issue, but I can investigate it further in another PR.

I implemented your other suggestions in regards to converting n and i into type Py_ssize_t.

seberg
seberg approved these changes Mar 6, 2025
View reviewed changes
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, @lvllvl.

@seberg seberg changed the title BUG: numpy.asarray's boundary check fails with too large shape and got a segmentation fault BUG: sanity check __array_interface__ number of dimensions Mar 6, 2025
@seberg seberg merged commit 7f0ebda into numpy:main Mar 6, 2025
68 checks passed
@charris charris added 00 - Bug 09 - Backport-Candidate PRs tagged should be backported labels Mar 6, 2025
@lvllvl lvllvl deleted the issue-27949 branch March 6, 2025 12:43
charris pushed a commit to charris/numpy that referenced this pull request Mar 7, 2025
@lvllvl @charris
BUG: sanity check __array_interface__ number of dimensions (numpy…
72efa8e 
…#28407)

``__array_interface__`` should typically not have more dimensions than NumPy supports, but unlike other malformed interfaces, this should fail gracefully if someone were to pass more.
@charris charris mentioned this pull request Mar 7, 2025
Merged
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seberg seberg seberg approved these changes

Assignees
No one assigned
Labels
00 - Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lvllvl @seberg @charris