Skip to content
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

index out of bounds when accessing when accessing an element in an array #21152

Open
samy-00007 opened this issue Aug 20, 2024 · 5 comments
Open
Labels
bug Observed behavior contradicts documented or intended behavior

Comments

@samy-00007
Copy link

Zig Version

0.14.0-dev.1217+dffc8c44f

Steps to Reproduce and Observed Behavior

I apologize for the poor issue title.
Here is a minimal code that produces the error:

const std = @import("std");

 var gpa = std.heap.GeneralPurposeAllocator(.{}){};
 const allocator = gpa.allocator();
 var list = std.ArrayList(usize).init(allocator);

 pub fn main() !void {
     std.debug.print("{}\n", .{list.items[try addToList(2)]});
 }

 fn addToLtist(x: usize) !usize {
     const result = list.items.len;
     try list.append(x);
     return result;
 }

Observed behavior: thread 144618: index out of bounds: index 0, len 0

Expected Behavior

The array access should not fail, since the element exists before it is accessed.

It is even possible to log the length of the array before the panic by putting a block (for instance blk: { const r = try addToList(2); std.debug.print("len: {}", .{list.items.len}); break :blk r; } ).
I am guessing the out of bound detection code takes the length of the array before compute the result of addToList

@samy-00007 samy-00007 added the bug Observed behavior contradicts documented or intended behavior label Aug 20, 2024
@nektro
Copy link
Contributor

nektro commented Aug 20, 2024

I think whats happening is that list.items in main is a stale pointer because addToList causes the backing allocation to be reallocated in order to grow in size

@nektro
Copy link
Contributor

nektro commented Aug 20, 2024

more specifically:

  • list.items in main is &.{}
  • addToList causes it to be allocator.alloc(1) since list.capacity is 0
  • list.items.len is now 1 but the temporary had already been created

not sure this is a bug.

@nektro
Copy link
Contributor

nektro commented Aug 20, 2024

its exhibiting the same failure as

pub fn main() !void {
    const x = list.items; // { ptr: undefined, len: 0 } is on the stack
    const y = try addToList(2); // original list.items is updated
    std.debug.print("{}\n", .{x[y]}); // access and print old value
}

@alexrp
Copy link
Member

alexrp commented Aug 21, 2024

The order of evaluation is working the way I'd expect it to here.

@rohlem
Copy link
Contributor

rohlem commented Aug 21, 2024

I think I can see arguments for both evaluation orders: Reading list.items before it was updated is left-to-right reading order, reading it after it was updated might conserve stack space.
As-is the behavior of the code is underdefined, just as I believe the equivalent C code would be (those 2 expressions share the same sequence point -> are indeterminately sequenced).
If Zig's language specification is going to keep the evaluation order as loose as C (doesn't enforce more sequence points), then I think this is unpredictable behavior that we might want to point out via a safety check (panic in safe build modes) - see #1966 and #2301 .

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

No branches or pull requests

4 participants