Skip to content

std.Progress: line_upper_bound_len is incorrectly calculated #20161

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
squeek502 opened this issue Jun 2, 2024 · 0 comments · Fixed by #20166
Closed

std.Progress: line_upper_bound_len is incorrectly calculated #20161

squeek502 opened this issue Jun 2, 2024 · 0 comments · Fixed by #20166
Labels
bug Observed behavior contradicts documented or intended behavior standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@squeek502
Copy link
Collaborator

squeek502 commented Jun 2, 2024

Zig Version

master

Steps to Reproduce and Observed Behavior

Probably dumb way to reproduce the problem:

const std = @import("std");

pub fn main() !void {
    var draw_buffer: [1000]u8 = undefined;
    const root_node = std.Progress.start(.{
        .draw_buffer = &draw_buffer,
        .estimated_total_items = 0,
        .root_name = "crunching some numbers",
    });
    defer root_node.end();

    var threads: [20]std.Thread = undefined;
    var thread_count: usize = 0;
    defer {
        for (threads[0..thread_count]) |thread| {
            defer thread.join();
        }
    }

    for (&threads) |*thread| {
        thread.* = try std.Thread.spawn(.{}, slowWork, .{root_node});
        thread_count += 1;
    }
}

fn fakeWork(parent_node: std.Progress.Node, name: []const u8) void {
    const node = parent_node.start(name, 0);
    defer node.end();
    for (0..10) |_| {
        std.time.sleep(50 * std.time.ns_per_ms);
        node.completeOne();
    }
}

fn slowWork(parent_node: std.Progress.Node) !void {
    const node = parent_node.start("crunching some numbers", 0);
    defer node.end();

    for (0..10) |_| {
        const sub = node.start("crunching some numbers", 0);
        defer sub.end();
        const fake_worker = try std.Thread.spawn(.{}, fakeWork, .{ sub, "crunching some numbers" });
        defer fake_worker.join();
    }
}

should eventually hit something like:

thread 10660 panic: index out of bounds: index 1002, len 1000
C:\Users\Ryan\Programming\Zig\zig\lib\std\Progress.zig:1101:17: 0x38e188 in computeRedraw (maxprog.exe.obj)
        buf[i..][0..finish_sync.len].* = finish_sync.*;
                ^
C:\Users\Ryan\Programming\Zig\zig\lib\std\Progress.zig:463:40: 0x380d9e in updateThreadRun (maxprog.exe.obj)
        const buffer, _ = computeRedraw(&serialized_buffer);
                                       ^
C:\Users\Ryan\Programming\Zig\zig\lib\std\Thread.zig:408:13: 0x3748e9 in callFn__anon_7931 (maxprog.exe.obj)
            @call(.auto, f, args);
            ^
C:\Users\Ryan\Programming\Zig\zig\lib\std\Thread.zig:518:30: 0x36676e in entryFn (maxprog.exe.obj)
                return callFn(f, self.fn_args);
                             ^

There are two problems here:

  • The easy-to-fix problem is that the \n byte is not included in the calculation
  • The harder-to-fix problem is that each computed node will end up including a variable number of up_one_line sequences (but line_upper_bound_len only calculates the space for one):

zig/lib/std/Progress.zig

Lines 1093 to 1096 in 64ef45e

for (0..nl_n) |_| {
buf[i..][0..up_one_line.len].* = up_one_line.*;
i += up_one_line.len;
}

Expected Behavior

Index-of-out-bounds should not be possible

@squeek502 squeek502 added the bug Observed behavior contradicts documented or intended behavior label Jun 2, 2024
@Vexu Vexu added the standard library This issue involves writing Zig code for the standard library. label Jun 2, 2024
@Vexu Vexu added this to the 0.13.0 milestone Jun 2, 2024
squeek502 added a commit to squeek502/zig that referenced this issue Jun 2, 2024
…bound_len

The \r\n is necessary to get the progress tree to work properly in the old console when ENABLE_VIRTUAL_TERMINAL_PROCESSING and DISABLE_NEWLINE_AUTO_RETURN are set.

The line_upper_bound_len fix addresses part of ziglang#20161
andrewrk added a commit that referenced this issue Jun 2, 2024
andrewrk pushed a commit that referenced this issue Jun 2, 2024
…bound_len

The \r\n is necessary to get the progress tree to work properly in the old console when ENABLE_VIRTUAL_TERMINAL_PROCESSING and DISABLE_NEWLINE_AUTO_RETURN are set.

The line_upper_bound_len fix addresses part of #20161
andrewrk added a commit that referenced this issue Jun 2, 2024
andrewrk added a commit that referenced this issue Jun 2, 2024
Rexicon226 pushed a commit to Rexicon226/zig that referenced this issue Jun 9, 2024
…bound_len

The \r\n is necessary to get the progress tree to work properly in the old console when ENABLE_VIRTUAL_TERMINAL_PROCESSING and DISABLE_NEWLINE_AUTO_RETURN are set.

The line_upper_bound_len fix addresses part of ziglang#20161
Rexicon226 pushed a commit to Rexicon226/zig that referenced this issue Jun 9, 2024
ryoppippi pushed a commit to ryoppippi/zig that referenced this issue Jul 5, 2024
…bound_len

The \r\n is necessary to get the progress tree to work properly in the old console when ENABLE_VIRTUAL_TERMINAL_PROCESSING and DISABLE_NEWLINE_AUTO_RETURN are set.

The line_upper_bound_len fix addresses part of ziglang#20161
ryoppippi pushed a commit to ryoppippi/zig that referenced this issue Jul 5, 2024
SammyJames pushed a commit to SammyJames/zig that referenced this issue Aug 7, 2024
…bound_len

The \r\n is necessary to get the progress tree to work properly in the old console when ENABLE_VIRTUAL_TERMINAL_PROCESSING and DISABLE_NEWLINE_AUTO_RETURN are set.

The line_upper_bound_len fix addresses part of ziglang#20161
SammyJames pushed a commit to SammyJames/zig that referenced this issue Aug 7, 2024
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 standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants