slice len unchanged after Allocator.resize is a footgun

[Pyrolistical]

Zig Version

0.12.0-dev.3609+ac21ade66

Steps to Reproduce and Observed Behavior

Given repo.zig:

const std = @import("std");

test {
    var buffer = try std.testing.allocator.alloc(u8, 3);
    defer std.testing.allocator.free(buffer);

    // realloc works as expected
    buffer = try std.testing.allocator.realloc(buffer, 2);
    try std.testing.expectEqual(2, buffer.len);

    std.debug.assert(std.testing.allocator.resize(buffer, 1));
    try std.testing.expectEqual(1, buffer.len);
}

Run zig test repo.zig:

Test [1/1] test_0... expected 1, found 2
[gpa] (err): Allocation size 1 bytes does not match free size 2. Allocation:
...\repo.zig:4:49: 0xdd118c in test_0 (test.exe.obj)
    var buffer = try std.testing.allocator.alloc(u8, 3);
                                                ^
[truncated...]

...\repo.zig:5:37: 0xbb12eb in test_0 (test.exe.obj)
    defer std.testing.allocator.free(buffer);
                                    ^
[truncated...]

...\repo.zig:12:5: 0x5a12e4 in test_0 (test.exe.obj)
    try std.testing.expectEqual(1, buffer.len);
    ^

I expected resize to have updated buffer.len, but it does not. This makes resize a footgun as buffer[1] will now segfault.

Also note how the free failed. Even the allocator expected a slice of len 1.

The following use case is how I encountered this issue:

extern fn foo(array_ptr: [*c]u8, array_capacity: u32, bytes_written: *u32) callconv(.C) void;

fn allocFoo(allocator: std.mem.Allocator, capacity: u32) ![]u8 {
  var buffer = try allocator.alloc(u8, capacity);
  errdefer allocator.free(buffer);

  var bytes_written: u32 = 0;
  foo(buffer.ptr, capacity, &bytes_written);
  if (!allocator.resize(buffer, bytes_written)) {
    buffer = try allocator.realloc(buffer, bytes_written);
  }
  return buffer;
}

test {
   const bytes = try allocFoo(std.testing.allocator, 10);
   defer std.testing.allocator.free(bytes);
   for (bytes) |_| {
     // segfaults if bytes_written is less than 10 and resize returned true
   }
}

The workaround is to use realloc instead.

Expected Behavior

To not shoot myself in the foot.

[Pyrolistical]

I understand resize could have never modified len of the passed in buffer, but I think this footgun still stands. resize should be reworked to return a new slice if it succeeded.

pub fn resize(self: Allocator, old_mem: anytype, new_n: usize) ?@TypeOf(old_mem);

[Pyrolistical]

Another way to see the footgun how we need to change the test to get it to pass using the existing resize api:

const std = @import("std");

test {
    var buffer = try std.testing.allocator.alloc(u8, 3);
    defer std.testing.allocator.free(buffer);

    // realloc works as expected
    buffer = try std.testing.allocator.realloc(buffer, 2);
    try std.testing.expectEqual(2, buffer.len);

    std.debug.assert(std.testing.allocator.resize(buffer, 1));
    buffer = buffer[0..1];  // added this line
    try std.testing.expectEqual(1, buffer.len);
}

Why is that so bad? Because in general a freeing a slice of allocated memory will cause a memory leak.

Consider the founding use case. Naively we could have written it without resize:

fn allocFoo(allocator: std.mem.Allocator, capacity: u32) ![]u8 {
  var buffer = try allocator.alloc(u8, capacity);

  var bytes_written: u32 = 0;
  foo(buffer.ptr, capacity, &bytes_written);
  
  // THIS WILL LEAK MEMORY
  return buffer[0..bytes_written];
}

As it stands now the footgun rule would be:

never return a slice of allocated memory, unless you resized it

[Trevor-Strong]

resize doesn't take a pointer to the slice, so it can't change the len of the buffer variable. All resize does is change the length of allocation (assuming it returns true), the caller is expected to set the length of their slice themselves, either with buffer = buffer[0..new_len] as you show, or just buffer.len = new_len

[andrewrk]

working as designed. The bool return value is the safety switch on the gun. Turn it off when you're aiming at your target.

[Pyrolistical]

I don't understand how I avoided/abused the safety switch

[andrewrk]

The assertion was just for demonstration purposes. In real code you would branch on it, which is your opportunity to edit the buffer:

-    std.debug.assert(std.testing.allocator.resize(buffer, 1));
+    if (std.testing.allocator.resize(buffer, 1)) {
+        buffer = buffer.ptr[0..1];
+    }

it's not a footgun in real world code:

zig/lib/std/array_list.zig

Lines 188 to 189 in f1eed99

	if (self.allocator.resize(old_memory, new_capacity)) {
	self.capacity = new_capacity;