Repository navigation
BUG: avoid incorrect stringdtype allocator sharing from array copies - #25944
Merged
Merged
Conversation
ngoldbaum
commented
Mar 5, 2024
| PyArray_ResolveWritebackIfCopy(obj); | ||
| if (PyArray_ResolveWritebackIfCopy(obj) < 0) { | ||
| goto fail; | ||
| } |
Member
Author
There was a problem hiding this comment.
This is an unrelated fix for a preexisting bug I ran into during debugging. The error value was discarded here and this function can error (it did in my testing) so we should have error checking.
ngoldbaum
commented
Mar 5, 2024
| npy_string_arena *arena = &in_allocator->arena; | ||
| if (arena->buffer == NULL) { | ||
| return -1; | ||
| } |
Member
Author
There was a problem hiding this comment.
this check is unnecessary, all the code below it can run even if arena->buffer is NULL
charris
approved these changes
Mar 7, 2024
mhvk
approved these changes
Mar 7, 2024
mhvk
left a comment
mhvk
left a comment
Contributor
There was a problem hiding this comment.
Sorry for the delay - this looks all good, and indeed makes things more logical!
| { | ||
| _npy_static_string_u *out_u = (_npy_static_string_u *)out; | ||
| unsigned char *flags = &out_u->direct_buffer.size_and_flags; | ||
| if (*flags == 0 || *flags & NPY_STRING_OUTSIDE_ARENA) { |
Contributor
There was a problem hiding this comment.
It had bothered me that this seemed necessary - great that it is not!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes the issue noted at #25890 (comment) by @mhvk.
The problem ultimately came down to passing the
allocatortonew_stringdtype_instanceinstringdtype_finalize_descr.I originally added that because I thought it had something to do with views, and I set
array_owned=2in that case. This was wrong, you can only ever get to this code path if you're creating a new array, so it's wrong to ever share allocators if you're callingnew_stringdtype_instance. As a result, I removed the third argument from that function and in all call sites and made it soarray_ownedcan only be 0 or 1. I also had a buggy incorrect default value ofarray_owned=2, which I delete here and leave it as zero by default (i.e. not yet owned by an array).This revealed a small bug in the stringdtype allocator implementation. Empty uninitialized strings set via
NpyString_pack_emptywould end up marked as initialized empty strings. The fix there is to delete the check forflags==0before setting the initialized flag in that case. As a result there are some changes toTestImplementation, since now empty uninitialized strings do not have the initialized and outside arena flags set.