Skip to content

BUG: fix memory leaks in init_iter_all when called on non C-contiguous arrays#545

Open
neutrinoceros wants to merge 2 commits intopydata:masterfrom
neutrinoceros:bug/528-leaks
Open

BUG: fix memory leaks in init_iter_all when called on non C-contiguous arrays#545
neutrinoceros wants to merge 2 commits intopydata:masterfrom
neutrinoceros:bug/528-leaks

Conversation

@neutrinoceros
Copy link
Copy Markdown
Collaborator

close #528 as suggested.
This is extremely similar to #534, with the difference that I don't know how to add a direct test for it.

Copy link
Copy Markdown
Collaborator

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This is modifying the wrong file. The two headers in bottleneck/include/ are outdated copies of the ones in bottleneck/src/, they're unused and should simply be deleted.

The PyArray_Ravel usage on line 148 seems to have the same issue.

return NULL isn't correct, because this is a static inline void function. This looks like it requires larger changes, changing the return type to int with return 0; at the end of the function and if (tmp == NULL) return -1; if the ravel call fails.

@neutrinoceros
Copy link
Copy Markdown
Collaborator Author

This is modifying the wrong file. The two headers in bottleneck/include/ are outdated copies of the ones in bottleneck/src/, they're unused and should simply be deleted.

oh, thanks for catching that, I was completely unaware.

The PyArray_Ravel usage on line 148 seems to have the same issue.

right, no idea how I missed that.

return NULL isn't correct, because this is a static inline void function. This looks like it requires larger changes, changing the return type to int with return 0; at the end of the function and if (tmp == NULL) return -1; if the ravel call fails.

It looked funky to me, but seeing it didn't break compilation I just jumped to the conclusion that my understanding of the signature must have been wrong. Instead, I should have realised I was editing a file that is not compiled at all...

Thanks for catching all of this, I'll update the PR shortly !

@neutrinoceros neutrinoceros marked this pull request as draft April 3, 2026 09:11
@neutrinoceros
Copy link
Copy Markdown
Collaborator Author

I hit a deterministic segfault, which is fixed by dropping calls to Py_DECREF(a). This was a shot in the dark, but then I studies PyArray_Ravel's definition an now it looks to me like the only case where the ref count is actually incremented is for order == NPY_KEEPORDER && !(PyArray_IS_C_CONTIGUOUS(arr)) && !( PyArray_IS_F_CONTIGUOUS(arr)), which shouldn't happen here, so Py_DECREF(a) was always unneeded and an incorrect suggestion from #521 (followed in #534) ?

@neutrinoceros neutrinoceros marked this pull request as ready for review April 3, 2026 09:30
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.

PyArray_Ravel unchecked in iterators.h init_iter_all — NULL dereference and ref leak

2 participants