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

std.fs.File.openRead() affects unrelated ArrayList #3630

Closed
joachimschmidt557 opened this issue Nov 8, 2019 · 8 comments
Closed

std.fs.File.openRead() affects unrelated ArrayList #3630

joachimschmidt557 opened this issue Nov 8, 2019 · 8 comments

Comments

@joachimschmidt557
Copy link
Member

Loading a file with std.fs.File.openRead() somehow affects an ArrayList not directly related to this File.

I've attached two files for reproducing: main.zig produces correct behaviour because the file loading is done after the clearing of the array. main2.zig doesn't exhibit correct behaviour and produces an index out out bounds error because of the wrong size (the program tries to clear an already empty ArrayList because the size has somehow been affected).

Output of main:

size after init 0
size at start of load 0
size after load 3

Output of main2:

siz after init 0
size after file load 3
index out of bounds
/home/joachim/zig/build/lib/zig/std/array_list.zig:58:30: 0x22b082 in std.array_list.AlignedArrayList(*std.array_list.AlignedArrayList(u8,null),null).toSliceConst (main2)
/home/joachim/zig/build/lib/zig/std/array_list.zig:63:37: 0x229f0f in std.array_list.AlignedArrayList(*std.array_list.AlignedArrayList(u8,null),null).at (main2)
/home/joachim/zig/build/lib/zig/std/array_list.zig:243:39: 0x229a7e in std.array_list.Iterator.next (main2)
/home/joachim/asdf/main2.zig:36:25: 0x2294e1 in load (main2)
/home/joachim/asdf/main2.zig:74:13: 0x228c25 in main (main2)
/home/joachim/zig/build/lib/zig/std/special/start.zig:242:37: 0x227a25 in std.special.posixCallMainAndExit (main2)
/home/joachim/zig/build/lib/zig/std/special/start.zig:106:5: 0x22789f in std.special._start 

main.zig:

...
fn load(state: *State, path: []const u8) !void {
    std.debug.warn("size at start of load {}\n", state.buffer.count());

    if (state.buffer.count() > 0) {
        var iter = state.buffer.iterator();
        while (iter.next()) |line| {
            line.deinit();
        }
    }
    state.buffer.deinit();

    const file = try std.fs.File.openRead(path);
    defer file.close();
...

main2.zig:

...
fn load(state: *State, path: []const u8) !void {
    const file = try std.fs.File.openRead(path);
    defer file.close();


    std.debug.warn("size after file load {}\n", state.buffer.count());

    if (state.buffer.count() > 0) {
        var iter = state.buffer.iterator();
        while (iter.next()) |line| {
            line.deinit();
        }
    }
    state.buffer.deinit();
...

Attachments: zig.zip

@andrewrk
Copy link
Member

andrewrk commented Nov 8, 2019

Did you perhaps capture references to array list elements, and then perform an operation that changes the size, such as append() ?

@joachimschmidt557
Copy link
Member Author

@andrewrk After clearing the array and loading the file, the code tries to read the file line by line into the ArrayList.

@joachimschmidt557
Copy link
Member Author

The same problem appears when I remove all code modifying the array:

const std = @import("std");

const Buffer = std.ArrayList(*std.ArrayList(u8));

const State = struct {
    allocator: *std.mem.Allocator,
    buffer: *Buffer,

    const Self = @This();

    fn init(alloc: *std.mem.Allocator) State {
        return Self {
            .allocator = alloc,
            .buffer = &Buffer.init(alloc),
        };
    }
};

fn load(state: *State, path: []const u8) !void {
    const file = try std.fs.File.openRead(path);
    defer file.close();


    std.debug.warn("size after file load {}\n", state.buffer.count());

    if (state.buffer.count() > 0) {
        var iter = state.buffer.iterator();
        while (iter.next()) |line| {
            line.deinit();
        }
    }
    state.buffer.deinit();
}

pub fn main() !void {
    var arena = std.heap.ArenaAllocator.init(std.heap.direct_allocator);
    defer arena.deinit();
    const allocator = &arena.allocator;

    var state = State.init(allocator);

    std.debug.warn("siz after init {}\n", state.buffer.count());

    try load(&state, "foo.txt");
    std.debug.warn("size after load finished {}\n", state.buffer.count());
}

Assuming the file foo.txt exisits, this code fails.

@andrewrk
Copy link
Member

andrewrk commented Nov 8, 2019

Have you tried running the code in valgrind?

@joachimschmidt557
Copy link
Member Author

Nope, not yet

@joachimschmidt557
Copy link
Member Author

Ok, I don't have a lot of experience with valgrind, but valgrind just gives me a lot of Conditional jump or move depends on uninitialised value(s) mainly for std.debug.warn but also for the if (state.buffer.count() > 0) in question.

But as far as I can tell, the ArrayList should be initialized with an empty slice and a length with 0 already through State.init() (as the program outputs siz after init 0).

@andrewrk
Copy link
Member

andrewrk commented Nov 9, 2019

All those errors from valgrind are true positives. In particular, you're returning a pointer to stack memory, which is becoming invalid upon return from the init() function.

This is one of the safety issues I'm planning to address in this release cycle. This issue is a duplicate of #3180.

@andrewrk andrewrk closed this as completed Nov 9, 2019
@joachimschmidt557
Copy link
Member Author

Oh okay, my bad. For a moment there, I forgot the init() function of an ArrayList (or in general) doesn't actually allocate the struct holding the data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants