Hejsil / zig-clap

Command line argument parsing library
MIT License
939 stars 67 forks source link

All parameter values get overwritten with 0xaa on Windows #43

Closed omgitsmoe closed 3 years ago

omgitsmoe commented 3 years ago

Using the clap.parse example verbatim all parameter values get printed as ¬, since the []u8 get overwritten with 0xaa for some reason, when appending them to the appropriate ArrayList in the loop at clap/comptime.zig:69. The example code also infinite loops when trying to print the multiple value param.

zig.exe build run -- test abc -n 5 -s str sdkf
--number = ¬
--string =

(Also tested it on Ubuntu 20.04 LTS (using WSL2) and it works there)

EDIT: Just a single option parameter seems to work:

zig.exe build run -- -n 512
--number = 512

EDIT2: Changing clap.parse so that parseEx does not reuse OsIterator's allocator solves the issue somehow.

Hejsil commented 3 years ago

Sorry for later reply. I must have marked this as read without actually seeing it.

This is a nasty bug. Even more nasty that it works on Linux. The problem here is that we copy the arena and pass that copied reference into parseEx. This results in us passing two arenas that share some state, but not all of it. The patch below should fix it (tested through wine). I'll open a PR later for this and tag a 0.4.1 release.

diff --git a/clap.zig b/clap.zig
index ee713a7..5ae1aec 100644
--- a/clap.zig
+++ b/clap.zig
@@ -294,17 +294,18 @@ pub fn parse(
 ) !Args(Id, params) {
     var iter = try args.OsIterator.init(opt.allocator);
     var res = Args(Id, params){
-        .arena = iter.arena,
         .exe_arg = iter.exe_arg,
+        .arena = undefined,
         .clap = undefined,
     };

     // Let's reuse the arena from the `OSIterator` since we already have
     // it.
     res.clap = try parseEx(Id, params, &iter, .{
-        .allocator = &res.arena.allocator,
+        .allocator = &iter.arena.allocator,
         .diagnostic = opt.diagnostic,
     });
+    res.arena = iter.arena;
     return res;
 }
omgitsmoe commented 3 years ago

I applied royghly the same fix and already wanted to submit a PR, but then I ran into a really weird bug where a stack-allocted variable or just entering a function would overwrite part of the arena's state struct.

This fails when trying to deinit the args struct, since part of the arena's state gets overwritten: grafik

PS D:\SYNC\coding\SciMarkdown\deps\zig-clap> .\zig-cache\o\32597f82d4f532e61e93ef696692277f\simple.exe tests -n 543
--number = 543
tests
Segmentation fault at address 0x60
D:\SYNC\coding\compilers\zig-windows-x86_64-0.8.0\lib\std\heap\arena_allocator.zig:47:33: 0x7ff67908188d in std.heap.arena_allocator.ArenaAllocator::std.heap.arena_allocator.ArenaAllocator.deinit (simple.obj)
            const next_it = node.next;
                                ^
D:\SYNC\coding\SciMarkdown\deps\zig-clap\clap.zig:257:27: 0x7ff67907c809 in .clap.Args(.clap.Help,[]const .clap.Param(.clap.Help){(struct .clap.Param(.clap.Help) constant),(struct .clap.Param(.clap.Help) constant),(struct .clap.Param(.clap.Help) constant),(struct .clap.Param(.clap.Help) constant)})::.clap.Args(.clap.Help,[]const .clap.Param(.clap.Help){(struct .clap.Param(.clap.Help) constant),(struct .clap.Param(.clap.Help) constant),(struct .clap.Param(.clap.Help) constant),(struct .clap.Param(.clap.Help) constant)}).deinit (simple.obj)
            a.arena.deinit();
                          ^
D:\SYNC\coding\SciMarkdown\deps\zig-clap\example\simple.zig:26:22: 0x7ff679063959 in main (simple.obj)
    defer args.deinit();
                     ^

Did I just mess sth. up or is this a compiler bug?

EDIT: Your fix works fine btw!

Hejsil commented 3 years ago

Did I just mess sth. up or is this a compiler bug?

Yep, you return a pointer to the arena. But the arena is stored on the stack frame of parse in iter, so the pointer is invalid once returned.

EDIT: Your fix works fine btw!

Nice, I'll push this fix soon™

omgitsmoe commented 3 years ago

Oh, I'm an idiot! I didn't even check if the arena is also stored on the stack (Should be obvious since it got overwritten by other stack-allocated values) 🤦