Temporaries and Scope Lifetime for Allocator Interfaces

Brainstorming
,
1

This would be broken. The FixedBufferAllocator has nowhere to live. The buffer itself is not enough, the FixedBufferAllocator needs somewhere to keep its data. Once the expression ends, the allocator would have a dangling pointer. The constness of temporaries just saved you from a footgun.

2 Likes
2

A post was merged into an existing topic: Result Location Semantics

3

i don’t see the footgun. what’s is it? allocator() has a pointer to value returned by init(), no?

there is a distinction between unnamed temporaries and named temporaries that zig lacks, and would be nice to have.

4

The FixedBufferAllocator stores state:

pub const FixedBufferAllocator = struct {
    end_index: usize,
    buffer: []u8,

// ...

By doing this:

const alloc = FixedBufferAllocator.init(&buf).allocator();

The actual “allocator” can go out of scope and then alloc can have a dangling reference to a dead object. If you’re using it all in the same scope then this probably works but in general you can end up using invalid stack memory if the allocator’s back-end goes out of scope.

5

Of course, that’s true of anythign.

The post just makes it seem like it was a guaranteed use after free or leak. That pattern is used all the time for passing a buffer down the call stack though and is no different that breaking it into two lines.

I thought the post was saying it was a prime facie bug, which I didn’t see.

6

It is a guaranteed user-after-free. The scope of a temporary is the expression where it lives. The original code:

var buf: [256]u8 = undefined;
const alloc = FixedBufferAllocator.init(&buf).allocator();

Is equivalent to this:

var buf: [256]u8 = undefined;
const alloc = blk: {
    var fba = FixedBufferAllocator.init(&buf);
    break :blk fba.allocator();
};

Once blk ends, fba goes out of scope, and the allocator contains a dangling pointer.

7

I still don’t see how that is true. You added a scope to a snippet of code that changed its semantics. The code you wrote does indeed have a bug, but that isn’t the code that was written and could have just as easily been at the top of a function preparing to pass it down the stack.

If it is just being passed down the stack, wihtout the block you introduced, is there a bug im missing?

1 Like
8

I’m moving these to a new topic so we can keep this thread about result location semantics. We can continue talking over here - I think this is a question best left to looking at the assembly.

9

The two code snippets have identical semantics. The scope of a temporary is just the expression where it was created, i.e, that one “line” of code until you reach the ;.
This is inherited from C and C++. There’s a bug in C++ that is almost a rite of passage for anyone writing concurrent code:

std::mutex m;
void func(){
    std::lock_guard(m);
    // Do concurrent work.
}

We naively assume that lock_guard will protect the entire scope, but it doesn’t. Once you reach the ;, the scope of the lock_guard ends, and the rest of func will execute without a lock. In order for this to work, you have to name it:

std::mutex m;
void func(){
    std::lock_guard guard(m);
    // Do concurrent work.
}

Now that it has a name (guard), it will last for the entire scope.
C++ even has the rule that a reference binding to a temporary extends the scope of it, but not even that you help you in the case we’re discussing.

const alloc = FixedBufferAllocator.init(&buf).allocator();
  1. init creates one temporary, we’ll call it fba.
  2. fba then creates another temporary, we’ll call it al.
  3. We reach the ;. The last temporary gets copied to alloc.
  4. The temporaries are deleted (fba and al).
  5. alloc now points to a dead variable.

The steps that I described are identical to the other snippet that I wrote, hence why these two are identical rewrites of each other:

var buf: [256]u8 = undefined;
const alloc = FixedBufferAllocator.init(&buf).allocator();

var buf: [256]u8 = undefined;
const alloc = blk: {
    var fba = FixedBufferAllocator.init(&buf);
    break :blk fba.allocator();
};
1 Like
10

What is the al temporary?

I see what you are talking about now, and I’ think you’re right. But the chain of copies doesn’t quite make sense to me. I don’t think it is the same as the C++ code because you there is no where to bind the value (you can take the address of a temporary reference for example).

  • init returns an FBA → fba
  • allocator take the address of fba (the signature is const ptr)

if fba is released at the end of the statement, then you’re right there is a problem. In c++ creating a reference toa temporary extends the lifetime of the obejct to that of the reference. Zig has no references, so it is hard to make the comparison.

Zig can either extend the lfietime of the object to that of the pointer (here, the end of block/fuinction) or not.

where is the al copy?

1 Like
11

al is a temporary of type Allocator, created by the temporary fba (which has type FixedBufferAllocator). When al was created, fba was in scope. In step 3:

  1. We reach the ;. The last temporary gets copied to alloc.

al gets copies to alloc. So the contents of alloc are pointing to a temporary variable.
In step 4:

  1. The temporaries are deleted (fba and al).

So now we have a dangling pointer.

12

Since it is bound, I don’t think it matters, but also it depends on how Zig treats RVO. The copy from allocate would still reference the same pointer, and I’m not sure if the lifetime of fba is tied to the lifetime of al.

For example c++ guarantees RVO and it can be relied upon for correctness, not just simple copy elision. (It used to not always be that way). C++ also has references though so it can have slightly different semantics and string guarantees than zig around refs than zig can provide with pointers. I think the C++ comparisons might not be useful because of this and just be bringing in other concepts.

  1. The temporaries are deleted (fba and al).

So in c++ if you took a reference to the temporary, you would be fine. It would extend to the lifetime of the ref which would be the lifetime of the allocator (and there would be no copy al copy).

If zig aggressively ends the lifetime of fba after its address is taken, then there is definitely a problem. If zig extends its lifetime to that of the pointer (similar to how c++ would), then there isn’t one.

I can see both being valid ways to handle it – especially if RVO is a langauge guarantee like c++. But I have a feel it is more just copy ellision than a language feature, and you are probably right.

Here’s another question: if you semantically inlined init and/or allocate would that change anything?

13

Zig’s lifetimes are simple. The scope ends, the variable is dead, there’s no extending it. Inlining doesn’t change anything, as a new scope is created when the compiler is copy-pasting the contents of the called function, hence resulting in the same problem.
Allocating directly from the temporary doesn’t currently work because of the rule that temporaries are const, which is what motivated this thread.

Consider this snippet:

var buf: [256]u8 = undefined;
const data = FixedBufferAllocator.
    init(&buf).
    allocator().
    alloc(u8, 1);

The FixedBufferAllocator keeps state, so it requires a mutable pointer to work. Since this is a temporary, it’s const, and that would fail. If we allowed temporaries to be mutable, this would work. Zig originally was like this, but at some point it explicity forbade mutable temporaries. I don’t know the exact reasons for it, but one of the reasons, I think, is that Zig reserves the right to put temporaries somewhere else than the stack, like the .rdata section.
Some allocators are stateless, like the page_allocator. We could potentially allocate from them, even when they are const.
I think the Allocator interface demands a mutable pointer, even if the allocator itself is stateless. If that’s the case, then it would be another block on our way.
Finally, there’s the problem of freeing the memory later, since you got rid of the initial allocator that allocated the memory. In some cases, that could be fine, like the FixedBufferAllocator of the example, since the buffer itself will eventually go out of scope. Stateless allocators, like the page_allocator, would also not have a problem with this.

2 Likes
14

Does this mean that each top level statement (not in the global sense but as in the local sense of not having any sub-statements) has its own scope that contains all temporaries of it and its sub-statements?

I know semantic inline allows types to escape, but I wasn’t sure what else interacted outside the lexical block of the function. I remember a while ago there being a thread where an inlined version of a function returned 1,2,3… and the regular version returned 1,1,1… but I can’t remember the exact nature of it. But that also dealt with the function arguments and not the return value. I wish I could find thread again, but I dont even remember where it was (could have even been a bug report for all I know).

the rule that temporaries are const

I’ve hit this issue a couple time where the temporary is required to be passed into a function as *const but since it is only used for the function call, it would be fine, except for that restriction. I have a rust like iterator lib that generates the exact same code for ReleaseSmall, but is slightly off in ReleaseFast. One of the issues is the iterator chain
iter_slice(some_slice).map(f).filter(g).avg() all stateless iterators are fine and it comes out just like a for loop. As sone as you add a satefull iterator you need to break it into two lines const i = iter_slice(some_slice).map(f).window(3) and const j = i.map(avg)... since window requires memory. (that would essentially create a moving average with period 3). its sooo close damn it.

give the scope is tied lexically to the line, you’re definitely right, and I didnt understand the zig scope mechanics. thanks for explaining it though. will definitely help.

1 Like
15

I believe this is correct. Every statement is a scope in and of itself, containg all the temporaries that are used in that statement.

1 Like
16

This would just incorrect use of an FBA though. Obviously the stack can’t move up from the creation of the FBA without invalidating all the memory it points to. So yes, in the code I was talking about, the entire buffer gets used within the function call, and no memory so allocated is ever returned.

I sort of took that for granted because of it being table stakes for using the FBA correctly.

There’s no reason why I should need to provide an intermediate location for the FBA itself to live. That could be elided, and it would go on the stack, without a way to reference it, which I don’t happen to need.

There’s no “footgun” between the three-line and the two-line version of the code, the only difference is that Zig doesn’t happen to recognize what’s going on, yet. It easily could.

17

This is an extension of what I was describing by a single line, yes. It happens that doing this is useless, because you can’t continue to use the allocator, but that’s the only difference.

That was my central point, in fact. Specifically, I think the RLS calculation should be able to take into consideration the use of a value as mutable. If it’s legal to make that value a var, then it should cast it to one.

It might be worth revisiting that decision, though. If the RLS system is flexible enough to default to const but cast to var if the result demands it, then the var version isn’t going anywhere but the stack. The const one can be put anywhere, although I can’t imagine that actually applying to this example.

Zig isn’t a garbage collected language, so if a method chain produced some mutable temporaries, we know they aren’t getting GC’d upon because of a lack of assignment to a variable. So there’s nowhere else to put them other than on the stack. I don’t see a compelling reason why there can’t be stack allocations without a symbol pointing to that allocation. It isn’t “hidden” if it’s a clear consequence of the code.

Those would have the same scope as everything around them. In the example I gave this would create no new problems.