What makes "ban returning pointer to stack memory" difficult?

Explain
21

By the same token, most uncomplicated bugs of this type are shallow. I’ve certainly returned directly from a stack frame a few times, but the next thing I try to do with that code shows the mistake.

We do get new users here who are confused by how memory works in Zig and don’t understand what went wrong. But we shouldn’t optimize for that case either.

I believe calling this just ‘costly’ is a subjective underestimate, but there’s some space in the correctness / expense map which might be acceptable.

I have a concern with this which isn’t perhaps obvious. You mentioned GCC and Clang, which can catch some bugs like that, and issue warnings. Zig doesn’t have warnings.

Which raises a problem with catching some-but-not-all of something. Namely, the project has a goal to standardize the language, so there’s a basis for multiple implementations. This requires that any given text file either compiles, or it doesn’t, and that it behaves in substantially the same way.

This is a bad fit for a project like you’re suggesting, because the only way to get it is to draw an arbitrary and complex-to-describe line, saying a Zig compiler will catch every case like this and will permit any other bugs of this nature. Improving analysis further would be a nonstandard extension. The conformance suite would have to include non-catchable bugs, or an implementation could easily refuse to compile programs allowed by the standard.

I’d like Zig to have a whole pluggable validation system, some included with the compiler and others third party. This would be a great addition to something like that. But I don’t think it should be part of the specification for the compiler, and that means that the compiler itself should not be doing it.

4 Likes
22

Such bugs can hard to detect if the pointer returned is high up on the stack. Consider the following:

const std = @import("std");

fn foo(i: usize) []const u8 {
    if (i == 0) {
        var buffer: [64]u8 = undefined;
        return std.fmt.bufPrint(&buffer, "It's a cookbook!", .{}) catch unreachable;
    } else {
        return foo(i - 1);
    }
}

pub fn main() !void {
    for (1..16) |i| {
        std.debug.print("{s}\n", .{foo(i)});
    }
}

�����
�������
������
@����@����
It's a cookbook!
�����
It's a cookbook!
It's a cookbook!
It's a cookbook!
It's a cookbook!
It's a cookbook!
It's a cookbook!
It's a cookbook!
It's a cookbook!
It's a cookbook!

With a StackFallbackAllocator it happens quite easily, since allocations occur from the top down:

const std = @import("std");

var gpa = std.heap.DebugAllocator(.{}).init;

fn foo() []const u8 {
    var sfb = std.heap.stackFallback(64 * 1000, gpa.allocator());
    return std.fmt.allocPrint(sfb.get(), "It's a cookbook!", .{}) catch unreachable;
}

pub fn main() !void {
    std.debug.print("{s}\n", .{foo()});
    if (!gpa.detectLeaks()) {
        std.debug.print("No leaks!\n", .{});
    }
}

It's a cookbook!
No leaks!
2 Likes
23

I don’t think this is necessary. If your program is using dangling pointers, it’s undefined behavior, and the compiler can do whatever it wants. The compiler may choose to reject it, or it may choose to compile it into garbage. Either way, it’s still compliant. Note that we are not talking about valid programs here. If the compiler still compiles the code, whatever comes out of it is fundamentally invalid.
For standardization purposes, we can just say: “if the compiler can prove that every code path leads to undefined behavior, it needs to reject the program”. Different compilers will have different proving capabilities, some may compile it (it’s still a garbage program), and some don’t.

24

Zig isn’t doing undefined behavior. It’s doing illegal behavior, and some of the distinctions between these are clearer than others.

One of the known goals is “comptime-known illegal behavior will not compile”. But note that my simple examples are not comptime-known as that concept currently exists in the compiler. A change there would be to what comptime knowability means, as distinct from adding static checks of one sort or another.

Another being “runtime-known illegal behavior will panic in safe modes”, and as Andrew pointed out, adding poorly-behaved stack uses to the detection list is a goal.

I want to re-emphasize the distinction between comptime-known, and that which could be statically determined while compiling. They’re different concepts.

And again, I think having an entire validation mechanism built into the toolchain would be excellent, and really elevate Zig above other tools in the same design space (further elevate that is).

I also think “a Zig compiler will, or will not, compile a text file, and the same decision will be reached for any given text file” is a pretty good goal here. To me that’s the distinction between illegal and undefined in a nutshell. YMMV.

We’re not stuck trying to find common ground between a bunch of compilers written out in the wild, so the constraints the C committee operates under shouldn’t be propagated, given all the experience we’ve had with the downsides of that decision.

But validation is great, it’s open ended and there’s always more of it. Let’s have that by all means.

5 Likes
25

Inline functions are probably the only legitimate use case for returning pointers to local variables:

const std = @import("std");

inline fn number(i: usize) []const u8 {
    var buffer: [32]u8 = undefined;
    return std.fmt.bufPrint(&buffer, "{d}", .{i}) catch unreachable;
}

pub fn main() void {
    std.debug.print("{s} {s}\n", .{ number(123), number(456) });
}
1 Like
26

I had never thought of that. I’m not sure this is legal. It would be like opening a block in the caller function. After the return statement, the block closes, and buffer goes out of scope.

27

I believe that in the current implementation all local variables within a function stay valid throughout the body regardless of scope (i.e. they have separate stack memory), but I doubt that is guaranteed to stay that way.

28

This is why language specification is difficult.

I would say this should be legal, but it’s open to debate:

fn whatAboutThis() void {
    var slice: []u8 = undefined;
    {
        var buf: [10]u8 = undefined;
        for (0..10) |i| {
            buf[i] = '?';
        }
        slice = buf[0..10];
    }
    std.debug.print("{s}\n", .{slice});
}

test "what happens here?" {
    whatAboutThis();
}

Currently it is: this prints ?, and not \xaa, as one would expect if it were illegal behavior. I think that’s desirable, even if this specific example is not a good idea.

2 Likes
29

I believe the stack is not filled with 0xAA on scope exit, because that would be too costly, even for debug builds.

1 Like
30

If it’s too costly to catch it, then in my opinion it should be legal. I think it’s perfectly alright to respect the already-existing distinction between a stack frame and a scope, scopes being about names and control flow, and stack frames being about bytes.

The compiler is free to reuse memory if it can prove that a variable is no longer in play, and I don’t think this kind of memory crossing scopes poses much of a problem, if any, for that mechanism.

I think the goal of catching all runtime-known illegal behavior in safe modes is an excellent one, and it’s worth standardizing this behavior over. If it’ll burn you in fast modes, it should crash your program in safe ones.

I do understand the feeling that this violates a certain principle of encapsulation, and even feel it a bit. But when I really think about, well, stack frames and scopes aren’t the same things.

1 Like
31

It also does effectively nothing if the stack memory gets reused and overwritten immediately–which is typical for stack memory.

32

If that’s the case, I do not like this at all and consider this a major bug.

Closing a scope should absolutely invalidate the things allocated to the stack in that scope. Otherwise, you can blow your stack by simply by having a couple of scopes with arrays. This would be terrible for embedded.

That having been said, is that code really acting in a valid way, or is it just that the stack area hasn’t gotten reused yet?

Edit: Yeah, this looks like a bug. Godbolt produces “X…” and then “?..”. That’s really bad. It should absolutely reuse that memory.

const std = @import("std");

fn whatAboutThis() void {
    var slice: []u8 = undefined;
    {
        var buf: [10]u8 = undefined;
        for (0..10) |i| {
            buf[i] = '?';
        }
        slice = buf[0..10];
    }
    {
        var buf: [10]u8 = undefined;
        for (0..10) |i| {
            buf[i] = 'X';
        }
        std.debug.print("{s}\n", .{buf});
    }
    std.debug.print("{s}\n", .{slice});
}



pub fn main() void {
    whatAboutThis();
}
1 Like
33

That’s the correct behavior, it’s what you asked for, and you got it. Calling it buf twice just muddies the water, can’t tell if you were confused by that or it if was just a rhetorical flourish.

What you’re showing is that the compiler won’t reuse memory if that memory is live. That’s table stakes, of course it won’t.

You’re not showing that it won’t reuse memory if the region is no longer live. Given that it’s LLVM we’re talking about, that is not the bet I would make here.

2 Likes
34

Wait, what, no, that memory should be dead. I closed the scope. Once you hit the “}” I expect buf to go poof.

Now, it’s possible that Zig created a copy which it then passed out of the scope. That would be … odd … but might explain the behavior.

I’ll have to try this with pointers to see what is going on.

Edit:

Even if I remove the illegal behavior, it still doesn’t reclaim the memory:
Result:

u8@7ffe3c0ae516
u8@7ffe3c0ae576

const std = @import("std");

fn whatAboutThis() void {
    {
        var buf: [10]u8 = undefined;
        for (0..10) |i| {
            buf[i] = '?';
        }

        const bufptr: *u8 = @ptrCast(&buf);
        std.debug.print("{any}\n", .{bufptr});
    }
    {
        var buf: [10]u8 = undefined;
        for (0..10) |i| {
            buf[i] = 'X';
        }

        const bufptr: *u8 = @ptrCast(&buf);
        std.debug.print("{any}\n", .{bufptr});
    }
}



pub fn main() void {
    whatAboutThis();
}

I’ll file a bug.

1 Like
35

Compilers are obviously capable of such analysis. By-register argument passing wouldn’t work otherwise.

1 Like
36

It isnot a bug. Although having same name, the two bufs in different scope are different!
So they have different addresses.

Regards!

37

This is an incorrect assumption on your part, that the close of a scope invalidates memory allocated within that scope.

It doesn’t, this is a difference between function calls and scopes. Function calls create a new stack frame (conceptually at least), and scopes do not.

Now, it could work that way. But I think that would be a bad idea. Meanwhile, it does not work that way, so, it’s not a bug, it’s… a feature!

3 Likes
38

Here’s a worked example of the kind of thing which we want to be functional:

const thing: []u8 = block: {
    if (do_short) {
         var short_buf: [4]u8 = undefined;
         const len = takesShortBuf(&short_buf);
         break :block short_buf[0..len];
   else {
         var long_buf: [64]u8 = undefined;
         const len = takesLongBuf(&long_buf);
         break :block long_buf[0..len];
    }
};

As a worked example, it’s not meant to be some exceptional solution to real problems. It’s meant to resemble a kind of code which should not lead to a spurious bug, because there’s nothing wrong with it.

1 Like
39

If I understand correctly, you are assuming that a scope’s memory lives until the end of the call frame instead of that scope. However, I don’t believe that it supposed to be the case:

1 Like
40

As long as the behavior is specified, and the compiler adheres to that specification consistently, I think either choice is defensible.

I agree with @nektro, however, that making it work that way (which it does not currently) would break a lot of code. I don’t see great reasons for that code to get broken.