Skip to content

std.mem.Allocator.free: also account for sentinel if it's a pointer to an array #23020

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

Closed
wants to merge 1 commit into from

Conversation

BlockOG
Copy link

@BlockOG BlockOG commented Feb 26, 2025

Fixes #23019 by checking the sentinel if it's a pointer to an array

@andrewrk andrewrk self-requested a review February 27, 2025 06:35
@andrewrk
Copy link
Member

change looks sus, needs a better writeup

@BlockOG BlockOG changed the title std.mem.Allocator.free: fix freeing of single pointer to array with sentinel std.mem.Allocator.free: also account for sentinel if it's a pointer to an array Feb 27, 2025
@T-727
Copy link

T-727 commented Feb 27, 2025

I believe the correct fix would be at

return @as(cast_target, @ptrCast(slice))[0 .. slice.len * @sizeOf(std.meta.Elem(Slice))];

-    return @as(cast_target, @ptrCast(slice))[0 .. slice.len * @sizeOf(std.meta.Elem(Slice))];
+    return @as(cast_target, @ptrCast(slice))[0 .. (slice.len + @intFromBool(std.meta.sentinel(Slice) != null)) * @sizeOf(std.meta.Elem(Slice))];

before, after

@nektro
Copy link
Contributor

nektro commented Feb 27, 2025

agreed, cc #19984

@ifreund
Copy link
Member

ifreund commented Feb 27, 2025

The correct fix is deprecating mem.sliceAsBytes() in favor of @ptrCast() and adding mem.absorbSentinel, I'll open a PR doing this in a minute.

#22706 (comment)

@ifreund
Copy link
Member

ifreund commented Feb 27, 2025

Actually, I wasn't thinking clearly earlier. The change proposed by this PR is just plain incorrect. To free a pointer to an array Allocator.destroy() should be used, not Allocator.free(). The fact that it is not a compile error to pass a pointer to an array to Allocator.free() is a bug and I will fix that in my PR adding mem.absorbSentinel().

@ifreund ifreund closed this Feb 27, 2025
ifreund added a commit to ifreund/zig that referenced this pull request Mar 26, 2025
Currently the only function that handles sentinel terminated slices
properly is free. All other uses of mem.sliceAsBytes() in the allocator
interface lack proper handling of a possible sentinel.

This commit changes the Allocator interface to use @ptrCast() plus
the new mem.absorbSentinel() instead.

This also makes incorrectly passing a pointer to array to
Allocator.free() a compile error. The proper function to free a pointer
to an array is Allocator.destroy().

Reported-by: David Vanderson <[email protected]>
References: ziglang#19984
References: ziglang#22706
References: ziglang#23020
ifreund added a commit to ifreund/zig that referenced this pull request Mar 27, 2025
Currently the only function that handles sentinel terminated slices
properly is free. All other uses of mem.sliceAsBytes() in the allocator
interface lack proper handling of a possible sentinel.

This commit changes the Allocator interface to use @ptrCast() plus
the new mem.absorbSentinel() instead.

This also makes incorrectly passing a pointer to array to
Allocator.free() a compile error. The proper function to free a pointer
to an array is Allocator.destroy().

Reported-by: David Vanderson <[email protected]>
References: ziglang#19984
References: ziglang#22706
References: ziglang#23020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trying to free a pointer to array with sentinel fails with invalid free
5 participants