Skip to content

runtime segfault when returning a union with a pointer #3138

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
ntgraff opened this issue Aug 29, 2019 · 3 comments
Closed

runtime segfault when returning a union with a pointer #3138

ntgraff opened this issue Aug 29, 2019 · 3 comments
Labels
bug Observed behavior contradicts documented or intended behavior
Milestone

Comments

@ntgraff
Copy link

ntgraff commented Aug 29, 2019

minimal example:

const std = @import("std");
const Allocator = std.mem.Allocator;

const Union = union(enum) {
    Number: *u32,
};

fn allocFn(allocator: *Allocator, value: u32) !*Union {
    var num = try allocator.create(u32);
    num.* = value;
    return Union{ .Number = num };
}

test "alloc fn" {
    var s = try allocFn(std.heap.direct_allocator, 10);
}
@rohlem
Copy link
Contributor

rohlem commented Aug 29, 2019

Just looking at the code, I'd say the issue is that allocFn's return statement returns a Union object, while the function's return type is error union of pointer to Union. (Changing the return type to !Union should fix this.)
I assume Zig currently provides an implicit cast from T to *T, which will return the address to the constructed temporary on the stack, which is invalidated by the function returning.
If I am correct about this, this would fall under the effort to try detecting escaping dangling pointers to stack addresses, #2646 .

Maybe it would be an easier first step to disallow these conversions to pointer types (T -> *T, [N]T -> [*]T and []T) in return (and by extension also break) expressions? (note prefixing a & to opt-in is still an option in this scenario)

@ntgraff
Copy link
Author

ntgraff commented Aug 29, 2019

Oy, you're right, but in my actual project I didn't have the !*<type> incorrect, it was just !<type> and it still segfaults:

pub fn cons(allocator: *Allocator, left: Atom, right: Atom) !Atom {
    var pair = try allocator.create(Pair);
    pair.* = Pair{
        .left = left,
        .right = right,
     // Segfault ^ happens here, according to the stack trace
    };
    // Atom.Pair is of type *Pair
    return Atom{ .Pair = pair };
}

But the original example I had was incorrect, and compiles correctly when you do !Union instead of !*Union. I agree with you that the conversions should be disallowed though.

@ntgraff
Copy link
Author

ntgraff commented Aug 29, 2019

I did some more testing, the issue only seems to happen if the union has a field that is void, and the struct has at least 2 fields:

const std = @import("std");
const Allocator = std.mem.Allocator;

const Struct = struct {
    field: Union,
    f2: Union,
};

const Union = union(enum) {
    Num: u128,
    Struct: *Struct,
    Void,
};

fn allocFn(allocator: *Allocator, value: Union) !Union {
    var s = try allocator.create(Struct);
    s.* = Struct{ .field = value, .f2 = v2 };
    return Union{ .Struct = s };
}

test "alloc fn" {
    var s = try allocFn(std.heap.direct_allocator, Union{ .Num = 10 }, Union{ .Num = 11 });
}

This example works if the struct has only one field, or if the union doesn't have the Void field.

@andrewrk andrewrk added this to the 0.6.0 milestone Aug 30, 2019
LemonBoy added a commit to LemonBoy/zig that referenced this issue Sep 10, 2019
Make sure the resulting type is in-sync with the one produced and used
by LLVM.

Fixes ziglang#3138
@andrewrk andrewrk added the bug Observed behavior contradicts documented or intended behavior label Sep 27, 2019
@andrewrk andrewrk modified the milestones: 0.6.0, 0.5.0 Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants