Zig: design flaw: using a struct as an interface and @fieldParentPtr can lead to bad pointer dereference

[andrewrk]

Reported here: https://github.com/scurest/zimodre/commit/8df208d42421693492668befd7811ea2761aa95f#commitcomment-25444745

Any struct that uses @fieldParentPtr on an "interface" field relies on the API user not making a copy of the interface field.

[andrewrk]

One possible solution:

• Implement visibility modifiers on container members (see #569)
• Make the interface a private field; expose a public function to get the interface which returns a pointer to the interface.

[andrewrk]

Another possible solution:

• Currently, a = b kind of violates "no hidden control flow". Because if b is a struct, it will do a hidden memcpy.
• If you see a = b, it's not clear if you're copying a reference (pointer/slice), or copying bytes of data (byval struct / enum / array).

We could disallow = to copy structs/enums/arrays. You would have to do @memcpy(&a, &b, 1);.

With this change, plus #287 (well-defined copy elision semantics), zig's "no hidden control flow" guarantee would even include no hidden memcpy calls.

[dbandstra]

Idea:

• fixed modifier on container member
• It's a compile error to copy a fixed field out of a container
• fieldParentPtr only works with fixed

This comes close to promoting fieldParentPtr to full language feature, though.

[ul]

Currently, a = b kind of violates "no hidden control flow". Because if b is a struct, it will do a hidden memcpy.

This bitten me so many times this weekend when I started to experiment with Zig! Few specific examples:

const Phasor = struct {
    // ... fields declaration ...

    fn init(freq: *Signal, phase0: *Signal) {
        const phasor = UnicastPhasor(freq, phase0);
        const multicast = Multicast(&phasor.signal);

        return Phasor {
            // oops, it's another phasor now and original is gone after return,
            // multicast will segfault trying to use signal reference
            .phasor = phasor,
            .multicast = multicast,
        };
    }
}

fn writeCallback(...) {
    const userdata = @ptrCast(*UserData, @alignCast(@alignOf(UserData), outstream.userdata));
    var context = userdata.context;
    ...
    for (frame < frame_count) : (frame += 1) {
        ...
        // oops, next time writeCallback is called sample_number starts from zero again!
        context.sample_number += 1;
    }
}

    var zero = audio.signal.zero.signal;
    var freq = audio.signal.Constant.init(440).signal;
    // oops, despite of those signals' parent structures being initialized in the same stack frame
    // they are gone and 440 being captured in Constant is silently lost
    var sine = audio.signal.Sine.init(&freq, &zero); 
    var userdata = audio.stream.UserData.init(&sine.signal);

Perhaps I just need to practice more (just started to write in Zig), but it tricks me again and again in different forms. I started to recognize it and understand how to fix (all mentioned cases are fixed now), but it requires a lot of attention to not make such mistake.

[andrewrk]

Thanks for your input @ul. I agree that this is a big flaw in the language. It's a top priority to fix in 0.4.0.

[ghost]

I think it’s best to eliminate parentPointer as the preferred way to do interfaces because it’s hard to read, hard to write and inefficient/ not optimal because of pointer indirections which can be eliminated.

[tgschultz]

Another issue with current interfaces is that you lose error set information. However, the current alternatives to this pattern aren't very good either. You can use var everywhere, but that makes the code more difficult to read because you don't know what is expected (potentially solvable with #1268), or you can use a type-generator like ArrayList does, but that gets verbose:

var sis = SliceInStream.init(data[0..]);
var reader = StreamReader(SliceInStream).init(sis.stream, builtin.Endian.Big);
var bit_reader = BitReader(@typeOf(reader)).init(&reader, builtin.Endian.Little);

[andrewrk]

This is an important issue, but it's blocking on #287, so postponing it to 0.5.0.

[shawnl]

@dbandstra Your fixed proposal also makes static analysis for strict aliasing much easier (which is currently not a zig thing). Given that struct members are not ordered, the compiler can then put the fixed member first.

[daurnimator]

This is an important issue, but it's blocking on #287, so postponing it to 0.5.0.

With copy elision part one complete is this issue unblocked?

[andrewrk]

With copy elision part one complete is this issue unblocked?

Yes => #3803