Uncaught segfault in debug mode: Safety of enum fields in packed structs is circumvented by @bitCast

[kj4tmp]

Zig Version

0.14.0-dev.1511+54b668f8a

Steps to Reproduce and Observed Behavior

The following test fails:

test {
    const ExampleEnum = enum(u8) {
        one = 1,
    };
    const PackedStructContainingEnum = packed struct(u8) {
        my_enum: ExampleEnum,
    };

    const two: u8 = 2;

    const my_struct: PackedStructContainingEnum = @bitCast(two);

    try std.testing.expect(@intFromEnum(my_struct.my_enum) != 2);
}

jeff@jeff-debian:~/repos/zecm$ zig test src/test.zig 
2/2 test.test_1...FAIL (TestUnexpectedResult)
/home/jeff/.config/Code/User/globalStorage/ziglang.vscode-zig/zig_install/lib/std/testing.zig:546:14: 0x10d5fcf in expect (test)
    if (!ok) return error.TestUnexpectedResult;
             ^
/home/jeff/repos/zecm/src/test.zig:33:5: 0x10d600d in test_1 (test)
    try std.testing.expect(@intFromEnum(my_struct.my_enum) != 2);

Edit: (Thanks to @mnemnion ) Also this segfaults:

const TwoEnum = enum(u8) {
    one = 1,
    two,
};

const PackedTwo = packed struct {
    one_or_two: TwoEnum,
};

test "switch on bitcast" {
    const three: u8 = 3;
    const oops: PackedTwo = @bitCast(three);
    switch (oops.one_or_two) {
        .one => std.debug.print("fine\n", .{}),
        .two => std.debug.print("also fine\n", .{}),
    }
}

Expected Behavior

I expected a panic on the bitcast, or perhaps a compile error.

However, a compile error would make the embedded use case of packed structs directly representing memory registers harder to implement. Maybe require bitcasted enums to be non-exhaustive?

related:
#18462
#3647

[rohlem]

I expected a panic on the bitcast, or perhaps a compile error.

packed struct exists in the language to reinterpret an aggregate of several types at bit-granularity as a single type (behaving like the backing integer).
enum fields in packed structs are useful (even exhaustive ones), and I don't think an (exhaustive) enum field should block (or complicate) this intended behavior.
However, I do think that on the field access my_struct.my_enum it would be correct for the compiler to assert that the observed value is valid for its type, i.e. creating an invalid value should be illegal behavior.
(I wrote proposal #6784 for this some time ago.)

[mnemnion]

In general, @bitCast should be looked at as a move outside of the type system. A sort of promise to the compiler that you know what you're doing. So I don't think a compile error make sense, or additional requirements for a @bitCast like a non-exhaustive enum: it's not that kind of function, it's a sharp tool and it can cut you.

Your motivating example is probably not worth adding safety checks over IMHO: you turned it into an illegal enum, and back into a perfectly cromulent integer, without doing anything which would get you in trouble. But there are some consequences here which should be addressed nonetheless.

This segfaults:

const TwoEnum = enum(u8) {
    one = 1,
    two,
};

const PackedTwo = packed struct {
    one_or_two: TwoEnum,
};

test "switch on bitcast" {
    const three: u8 = 3;
    const oops: PackedTwo = @bitCast(three);
    switch (oops.one_or_two) {
        .one => std.debug.print("fine\n", .{}),
        .two => std.debug.print("also fine\n", .{}),
    }
}

It's hard to draw a clear line for how much responsibility the compiler should take when user code jukes the type system with a @bitCast, but "should panic rather than segfault" is probably inside that line.

Debatable whether debug-mode @bitCast should validate on the result location, or if it should add an implicit unreachable prong to exhaustive switches, but it should do something to catch this.

I'd suggest editing the title of this issue to have the word "segfault" in it. The example you picked is basically harmless, but it can have effects which are not.

[kj4tmp]

After thinking about this, I support at least panic on invalid values. The proposal @rohlem linked I think is good.

Here is a real-world use case code snippet from me https://github.com/kj4tmp/zecm:

I am de-serializing a byte stream containing many bit fields and sparse enums. This can be made more convenient by using packed structs containing enums. I am using the little endian layout of packed structs to define a binary protocol schema. I realize this might not be an intended use case for packed structs, but it is certainly convenient (and does not extend very well to big-endian protocols :).

I already expected a panic on invalid values and I have been very careful to make sure all my deserialized enums are non-exhaustive to force myself to handle invalid values from the byte stream.

/// convert a packed struct to bytes that can be sent via ethercat
///
/// the packed struct must have bitwidth that is a multiple of 8
pub fn eCatFromPack(pack: anytype) [@divExact(@bitSizeOf(@TypeOf(pack)), 8)]u8 {
    comptime assert(isECatPackable(@TypeOf(pack)));
    var bytes: [@divExact(@bitSizeOf(@TypeOf(pack)), 8)]u8 = undefined;
    switch (native_endian) {
        .little => {
            bytes = @bitCast(pack);
        },
        .big => {
            bytes = @bitCast(pack);
            std.mem.reverse(u8, &bytes);
        },
    }
    return bytes;
}

pub fn isECatPackable(comptime T: type) bool {
    if (@bitSizeOf(T) % 8 != 0) return false;
    return switch (@typeInfo(T)) {
        .@"struct" => |_struct| blk: {
            // must be a packed struct
            break :blk (_struct.layout == .@"packed");
        },
        .int, .float => true,
        .@"union" => |_union| blk: {
            // must be a packed union
            break :blk (_union.layout == .@"packed");
        },
        else => false,
    };
}

/// Read a packed struct, int, or float from a reader containing
/// EtherCAT (little endian) data into host endian representation.
pub fn packFromECatReader(comptime T: type, reader: anytype) !T {
    comptime assert(isECatPackable(T));
    var bytes: [@divExact(@bitSizeOf(T), 8)]u8 = undefined;
    try reader.readNoEof(&bytes);
    return packFromECat(T, bytes);
}

pub fn packFromECat(comptime T: type, ecat_bytes: [@divExact(@bitSizeOf(T), 8)]u8) T {
    comptime assert(isECatPackable(T));
    switch (native_endian) {
        .little => {
            return @bitCast(ecat_bytes);
        },
        .big => {
            var bytes_copy = ecat_bytes;
            std.mem.reverse(u8, &bytes_copy);
            return @bitCast(bytes_copy);
        },
    }
    unreachable;
}

[mnemnion]

This isn't really the right place for me to give feedback on the approach you're taking with the library, but there are ways to accomplish what you're trying to do which don't circumvent the type system.

Which why I suggest changing the title to reflect the fact that you (well, me, but there's no point filing a separate issue since it's the same problem) found a segfault in Debug mode.

@bitCast circumvents type safety, that's a known thing. It's a cast, casts all do that, it is, in fact, what @bitCast exists to do.

const long: u64 = @bitCast(@as(f64, 3.14159));

That circumvents type safety. It isn't clear from your title or example why this use of @bitCast should be considered a problem. #2301 is a relevant issue, and panicking instead of segfaulting is an example of the sort of thing which is supposed to happen when illegal behavior is exhibited. Round-tripping a number back and forth through a packed struct with an enum which can't hold that number is not itself the same kind of problem.

For example, if we change the test in my example to this:

test "switch on invalid enum" {
    const three: u8 = 3;
    const oops: TwoEnum = @enumFromInt(three);
    switch (oops.one_or_two) {
        .one => std.debug.print("fine\n", .{}),
        .two => std.debug.print("also fine\n", .{}),
    }
}

It returns the error error: enum 'example.TwoEnum' has no tag with value '3'. It's specifically the combination of using a cast and then switching on the result which segfaults, and I bet there are other examples of corrupted state which can result from this.

By extension, if there are illegal values for an enum in your packed structs, you can assign that enum to the field with @enumFromInt and get safety-checked illegal behavior right now.

[kj4tmp]

Which why I suggest changing the title to reflect the fact that you (well, me, but there's no point filing a separate issue since it's the same problem) found a segfault in Debug mode.

Thanks for the example! I edited the issue description and title.