Maybe a stdliib bug / now barely worth discussion

Help
1

I have a little chunk of zig code here whose goal is to open and just print out markdown files: https://codeberg.org/jackdev/mdt/src/commit/55bacc855ffa5c6bc572bbe66cbeb2c6663aed60/src/ref_check.zig#L38-L48.

The test-case, "it runs without errors", is not passing. I’m using zig 0.16.0-dev.2962+08416b44f and I have ZIG_LIB_DIR pointing to a checkout of e938344100da959308aa27dee1295e5ce02efc30 from https://codeberg.org/ziglang/zig. I get the following error, an integer overflow in the stdlib;

error: 'ref_check.test.it runs without errors' terminated with signal ABRT with stderr:
       thread 218293 panic: integer overflow
       /Users/johndevries/.local/quickzilver/lib/std/Io/Writer.zig:2730:67: 0x102ef46bf in sendFile (test)
               const additional = if (file_reader.getSize()) |size| size - pos else |_| std.atomic.cache_line;
                                                                         ^
       /Users/johndevries/.local/quickzilver/lib/std/Io/Writer.zig:920:29: 0x102df225f in sendFile (test)
           return w.vtable.sendFile(w, file_reader, limit);
                                   ^
       /Users/johndevries/.local/quickzilver/lib/std/Io/File/Reader.zig:207:53: 0x102df1a9f in streamMode (test)
               .positional, .streaming => return w.sendFile(r, limit) catch |write_err| switch (write_err) {
                                                           ^
       /Users/johndevries/.local/quickzilver/lib/std/Io/File/Reader.zig:202:22: 0x102debeef in stream (test)
           return streamMode(r, w, limit, r.mode);
                            ^
       /Users/johndevries/.local/quickzilver/lib/std/Io/Reader.zig:176:34: 0x102dee647 in stream (test)
           const n = try r.vtable.stream(r, w, limit);
                                        ^
       /Users/johndevries/.local/quickzilver/lib/std/Io/Reader.zig:261:27: 0x102f0067b in streamRemaining (test)
               offset += r.stream(w, .unlimited) catch |err| switch (err) {
                                 ^
       /Users/johndevries/.local/quickzilver/lib/std/Io/Reader.zig:401:24: 0x102ef5833 in appendRemainingUnlimited (test)
           _ = streamRemaining(r, &a.writer) catch |err| switch (err) {
                              ^
       /Users/johndevries/repos/mdt/src/ref_check.zig:45:52: 0x102f13f77 in ref_check (test)
                           try rd.appendRemainingUnlimited(alloc, &mem);
                                                          ^
       /Users/johndevries/repos/mdt/src/ref_check.zig:68:18: 0x102f1454f in test.it runs without errors (test)
           try ref_check(alloc, io, dir);
                        ^
       /Users/johndevries/.local/quickzilver/lib/compiler/test_runner.zig:147:68: 0x102ed21ff in mainServer (test)
                       const status: TestResults.Status = if (test_fn.func()) |v| s: {
                                                                          ^
       /Users/johndevries/.local/quickzilver/lib/compiler/test_runner.zig:68:26: 0x102ece7ab in main (test)
               return mainServer(init) catch |err| std.debug.panic("internal test runner failure: {t}", .{err});
                                ^
       /Users/johndevries/.local/quickzilver/lib/std/start.zig:678:88: 0x102ece29b in callMain (test)
           if (fn_info.params[0].type.? == std.process.Init.Minimal) return wrapMain(root.main(.{
                                                                                              ^
       ???:?:?: 0x18377b153 in start (/usr/lib/dyld)
failed command: ./.zig-cache/o/c98635e01ff2f743da833c59e816a01d/test --cache-dir=./.zig-cache --seed=0x88a61f12 --listen=-

Now, I went ahead and patched the stdlib as follows to fix the integer overflow;

diff --git a/lib/std/Io/Writer.zig b/lib/std/Io/Writer.zig
index 0df202e68f..7ee9f352d7 100644
--- a/lib/std/Io/Writer.zig
+++ b/lib/std/Io/Writer.zig
@@ -2727,7 +2727,19 @@ pub const Allocating = struct {
         if (limit == .nothing) return 0;
         const a: *Allocating = @fieldParentPtr("writer", w);
         const pos = file_reader.logicalPos();
-        const additional = if (file_reader.getSize()) |size| size - pos else |_| std.atomic.cache_line;
+        const additional = blk: {
+            if (file_reader.getSize()) |size| {
+                break :blk std.math.sub(
+                    u64,
+                    size,
+                    pos,
+                ) catch |e| switch (e) {
+                    error.Overflow => 0,
+                };
+            } else |_| {
+                break :blk std.atomic.cache_line;
+            }
+        };
         if (additional == 0) return error.EndOfStream;
         a.ensureUnusedCapacity(limit.minInt64(additional)) catch return error.WriteFailed;
         const dest = limit.slice(a.writer.buffer[a.writer.end..]);

However, even after “fixing” the “issue” in the stdlib, I don’t get expected output from my program. Instead, I get this –

processing README.md

processing images.md

processing basic.md

processing README.md

processing links.md

processing chapter1.md

processing section.md

processing chapter3.md

processing chapter2.md

failed command: ./.zig-cache/o/654786069fbbf6b52ea00b1479e2d620/test --cache-dir=./.zig-cache --seed=0xe3a6d409 --listen=-

In other words, it’s walking the file-system successfully but doesn’t appear to be reading any text in from any of the files. This makes me think that I’m using some or all of the APIs that are in play here incorrectly. At this point, I’m not really sure if this is a stdlib bug or if my API misuse is tantamount to fuzzing such that I’ve violated an invariant I don’t know about.

In any case, I think I’ve whittled this down to a minimal enough state to share and invite some discussion!

2

I can’t exactly reproduce your error (probably we use different OSs), but two things:

openDir options: .{} → .{ .iterate = true } (required on some targets for std.Io.Dir.walk)

var rd = file.reader(io, rd_buf).interface;
try rd.appendRemainingUnlimited(alloc, &mem);

→ 

var rd = file.reader(io, rd_buf);
try rd.interface.appendRemainingUnlimited(alloc, &mem);

The interface is not a simple value, which can be copied onto the stack. You can create a pointer to the interface field, but copying it onto the stack and then using it results in unexpected behavior.

4 Likes
3

Severing the interface from the reader instance created by file.reader() as this code does is definitely broken.

4 Likes
4

As @rpkak and, more precisely, @pachde pointed out: you are copying the interface, this causes illegal behaviour that is the source of your original error.

Looking at the original logic of the code you “fixed”, pos should never be larger than size, so size - pos should never panic. Your “fix” only allows the code to continue in an invalid state, caused by your own programmer error in your own code.

5

The OP is not only copying the interface, but copying it from a temporary value that “no longer exists” after the statement.

1 Like