Coercion to less aligned is not safe

[michal-dobrogost]

Zig Version

0.15.2

Steps to Reproduce and Observed Behavior

Coercing a more aligned type to a less aligned type is not safe as the memory is no longer able to be freed correctly. This coercion behaviour disables std.mem.alignedAlloc from enforcing that the returned type has the correct alignment (or flagging the risky behaviour via @alignCast).

This has hit multiple users (+1 for myself now) and is only detected by std.testing.allocator. Other allocators continue silently.
https://discordapp.com/channels/605571803288698900/1375373321691992126
https://discordapp.com/channels/605571803288698900/1421631270504632441

Repro:

test "alignedAlloc" {
    const gpa = std.testing.allocator;
    const alignment: std.mem.Alignment = @enumFromInt(4);
    const data: []u8 = try gpa.alignedAlloc(u8, alignment, 12);
    defer gpa.free(data); // [gpa] (err): Allocation alignment 16 does not match free alignment 1.
}

Expected Behavior

Any change in alignment requires @alignCast.

[Vexu]

This is the intended behavior #17668 (comment)

[michal-dobrogost]

Thanks for digging that up. It's a shame to accept the status quo here as the current behaviour as is so unexpected, silent and dangerous.

I can put up a PR to at least document the current behaviour.

[blblack]

Re-framing from the issue title here: fundamentally, going from a stricter alignment to a looser (smaller) alignment is safe. The problem is that free() expects to be given the same type that alignedAlloc() returned, and we've instead coerced it to be something else before we hand it back to free(). This alternative version works fine:

test "alignedAlloc" {
    const gpa = std.testing.allocator;
    const alignment: std.mem.Alignment = @enumFromInt(4);
    const raw_data = try gpa.alignedAlloc(u8, alignment, 12);
    defer gpa.free(raw_data); // no error, we gave back what we were originally given
    const coerced_data: []u8 = raw_data; // safe coercion to smaller alignment
    _ = coerced_data; // can be safely used
}

[michal-dobrogost]

We can't use your example to show that alignment coercian is safe because there is no alignment coercian in your code (but there is in the original example).

[michal-dobrogost]

FWIW here is the doc comment I have ready for alignedAlloc but it doesn't address the underlaying issue just helps prevent some people from blasting their feet off.

+/// Allocates with an alignment which is incorporated into the returned type.
+/// Call `free` when done.
+///
+/// Prefer inferred types like `const data = alignedAlloc(...)` as it will use
+/// the correctly aligned type.
+///
+/// Avoid explicit types like `const data: u8[] = alignedAlloc(...)` as they
+/// can change the alignment type information needed when calling `free`.
+///
+/// If alignment is not comptime known use `rawAlloc` and `rawFree`.

[blblack]

We can't use your example to show that alignment coercian is safe because there is no alignment coercian in your code (but there is in the original example).

The same coercion exists in both examples. In my version, I'm just not using the coerced copy of the pointer when I call free().

[blblack]

The earlier example, but with types made explicit:

test "alignedAlloc" {
    const gpa = std.testing.allocator;
    const alignment: std.mem.Alignment = @enumFromInt(4);
    const raw_data: []align(16) u8 = try gpa.alignedAlloc(u8, alignment, 12);
    defer gpa.free(raw_data); // no error, freed what we were actually given (align(16))
    const data: []align(1) u8 = raw_data; // coerces from align(16) to align(1) safely
    _ = data;
}

[michal-dobrogost]

Can you coerce and then pass into free safely?

[blblack]

Can you coerce and then pass into free safely?

No, not without coercing it back to the original type. That was the point I'm trying to re-frame here. The fundamental issue here is not really about coercion or @alignCast(), it's about the Allocator API requiring that free() be given the same type that an allocation function originally returned. The effective contract with the Allocator API is not just a pointer, it's a specific type of pointer.

[michal-dobrogost]

Exactly, free depends on the alignment information encoded in the type which combined with alignment coercion rules results in unsafe code.

The structure of your counter example can be used to make the statement "accessing undefined is defined":

test "definedUndefined" {
    var x: u64 = 42;
    std.debug.print("{}\n", .{x});
    x = undefined;
}

[michal-dobrogost]

With the distinction that the unsafe operation for undefined values is accessing them, and for alignment coerced values it is freeing them.

[blblack]

Not all values or types even interact with allocation at all. At a language level, IMHO, alignment casting and coercion are designed correctly. We don't want to have to explicitly downcast from greater to lesser alignment; that doesn't make fundamental sense in most cases. For example:

test "alignment" {
    var my_data: [16]u8 align(16) = @splat(0);
    my_data[12] = 12;
    const my_slice: []align(16) u8 = my_data[0..16];
    const where = std.mem.findScalar(u8, my_slice, 12);
    try std.testing.expect(where == 12);
}

findScalar() takes a normal []u8 argument here and doesn't need higher alignment to do its job. Just because I've aligned my data for some other reason, doesn't mean I should have to explicitly @alignCast() this every time I call some stdlib function that's fine with any alignment of []u8. In a very fundamental sense, coercing to lesser alignment is always ok. For example, align(16) satisfies a requirement of align(8) (but not the other way around!).

I'm asserting that your real issue here is not with the language, but with the Allocator interface, and specifically the fact that while free() takes an anytype, it actually requires a very specific type, which is the exact type originally handed out by its allocation function earlier when it allocated the pointer in question. The type info is carrying important infomation that the allocator implementation needs to know. That is a bit of a footgun in some senses, but it's an Allocator footgun, not a language footgun. I think if you tried to design a better generic Allocator interface that makes this a compile error, though, you'll find other unacceptable tradeoffs.

[michal-dobrogost]

Agreed - some code doesn't interact with allocation at all. I think that sidesteps the issue. A lot of (most?) code does and we should consider it.

IMO memory management is so fundemental that it is closely tied to the langauge itself (eg. zig defer, c++ destructors, java garbage collection, rust linear types etc). I have a hard time thinking about a langauge fully in isolation of memory management even if it's implmented in the std lib.

That aside, I think that the language is complicit in this issue. std.mem.Allocator couldn't do what it does without the interaction between alignment coercion rules and type reflection.

Alignment is unique among the coercion rules previously discussed in #17668 (comment). It has several unique characteristics (see code below):

(1) Alignment coercion is the only one where information is changed with abilities unaffected. The other coercion cases result in lost information or lost abilities.

(2) All other cases only exist at the language level but alignment also matters at the machine level. This means that some code will care about making use of alignment information. For example, SIMD cares about memory alignment and so an implementation can choose to use SIMD or not based on the alignment type info. This can result in a silent performance degradation when the language could have flagged the alignment typeinfo change to the reader instead.

test "coercion" {
    // Lost ability to modify the value.
    var v: u8 = 42;
    const p0:*u8 = &v;
    const p1:*const u8 = p0;
    std.debug.print("p0:{}\np1:{}\n\n", .{@typeInfo(@TypeOf(p0)), @typeInfo(@TypeOf(p1))});
    // .is_const = false
    //           = true

    // Lost information about error set
    const ErrorSet = error{ MyNewError };
    const e0: ErrorSet  = ErrorSet.MyNewError;
    const e1:anyerror = e0;
    std.debug.print("e0:{}\ne1:{}\n\n", .{@typeInfo(@TypeOf(e0)), @typeInfo(@TypeOf(e1))});
    // .error_set = { .{ .name = { ... } } }
    //            = null

    // Lost information about pointer type
    const o0:*anyopaque = p0;
    std.debug.print("p0:{}\no0:{}\n\n", .{@typeInfo(@TypeOf(p0)), @typeInfo(@TypeOf(o0))});
    // .child = u8
    //        = anyopaque

    // Changed existing alignment information.
    const a0: [1]u8 align(16) = @splat(v);
    const a1: *align(16) const u8 = &a0[0];
    const a2: *align(1) const u8 = a1;
    std.debug.print("a0:{}\na1:{}\n\n", .{@typeInfo(@TypeOf(a1)), @typeInfo(@TypeOf(a2))});
    // .alignment = 16
    //            = 1
}

Fundementally I have a hard time understanding why being explict about decreased alignment is a bad thing. I understand that for most access it doesn't matter but there are cases when it does. The burden seems minimal as you only have to pay the explicit tax if the code cares about alignment.

Funnily enough, reading through the zen of zig (+ claude's added blurb), I surprised myself how many items seem to apply. Perhaps I'm biased.

1. Communicate intent precisely - Avoid ambiguity and implicit behavior that forces readers to guess.
2. Edge cases matter - Don't ignore unusual or boundary conditions.
3. Favor reading code over writing code - Verbosity is acceptable if it aids understanding.
4. Compile errors are better than runtime crashes - Catch problems at compile-time whenever possible.
5. Reduce the amount one must remember - Minimize special cases, exceptions to rules, and gotchas.
6. Resource allocation may fail; resource deallocation must succeed

[castholm]

Other allocators continue silently.

I just want to point out that std.heap.DebugAllocator(.{ .safety = true }), which is the default in Debug/ReleaseSafe, logs error(gpa): Allocation alignment n does not match free alignment m and stack traces (but then continues like nothing happened which surprised me a bit, I would have expected a panic). The Zig modus operandi for protecting yourself from memory unsafety and leaks is by writing tests that use std.testing.allocator and by using a validating allocator like DebugAllocator in debug builds.

That said, I agree that it's a pitfall. However, consider if the signature of free() was changed to something like this:

fn freeWithOptions(
    allocator: Allocator,
    T: type,
    comptime alignment: ?Alignment,
    comptime sentinel: ?T,
    slice: FreeWithOptionsPayload(T, alignment, sentinel),
) void {
    allocator.free(slice);
}

fn FreeWithOptionsPayload(comptime Elem: type, comptime alignment: ?Alignment, comptime sentinel: ?Elem) type {
    if (sentinel) |s| {
        return [:s]align(if (alignment) |a| a.toByteUnits() else @alignOf(Elem)) const Elem;
    } else {
        return []align(if (alignment) |a| a.toByteUnits() else @alignOf(Elem)) const Elem;
    }
}

You are now protected against mistakes like these:

const data: []u8 = try gpa.allocWithOptions(u8, 10, .fromByteUnits(4), 0);
defer gpa.freeWithOptions(u8, .fromByteUnits(4), 0, data);

repro.zig:?:?: error: expected type '[:0]align(4) const u8', found '[]u8'

But in truth, we've kinda only really traded it for a different footgun:

// initial code
const data = try gpa.allocWithOptions(u8, 10, null, null);
defer freeWithOptions(gpa, u8, null, null, data);

// some time later we realize the data should be aligned and 0-terminated
const data = try gpa.allocWithOptions(u8, 10, .fromByteUnits(4), 0);
defer freeWithOptions(gpa, u8, null, null, data); // oops! forgot to change this line!

Now we're back to square one.

I don't know if it's fully possible to design an allocator API that doesn't have this problem in one form or another without removing sentinel/alignment-dropping coercions from the language.