Weird textInput buffer handling

[mifopen]

Hey!
I have noticed some weird behaviour of zgui.textInput that I do not understand.
Let's say we have the following code written in C/C++:

static char text[1024] = "";
static ImGuiInputTextFlags flags = ImGuiInputTextFlags_EnterReturnsTrue;
if(ImGui::InputText("##source", text, IM_ARRAYSIZE(text), flags)) {
  fprintf(stdout, "%s\n", text);
}

and now we want to convert it into Zig using zig-gamedev/zgui lib.

const static = struct {
    var buf: [128]u8 = undefined;
};
if (zgui.inputText(
    "##source",
    .{
        .buf = &static.buf,
        .flags = .{
            .enter_returns_true = true,
        },
    },
)) {
    std.debug.print("{s}\n", .{&static.buf});
}

Now if you do this:

1. Enter "hello"
2. Press Enter
3. See "hello" in terminal
4. Focus input again
5. ctrl+a / cmd+a (select text)
6. Type 'x'
7. Press Enter

For the C variant, you will see x in the terminal as expected
For the Zig variant, you will see xllo in the terminal.

It could be definitely that I don't understand some very basic stuff about system programming/C/Zig as I'm just entering this realm. Could you please help me understand what is going on here?

[hazeycode]

Hey @mifopen, this is a good report!

It looks like Dear ImGui only writes a single terminal \0 after the string, as opposed to zeroing the entire buffer. In C/C++, because there is no "default" bounds checking and because strings are assumed to be null-terminated, this behavior makes sense.

Note that in your C++ code, the print statement null-terminates the string. But in your Zig code, you are printing the entire buffer, nulls included. Instead, you could try using std.mem.sliceTo or casting to a null-terminated pointer type.

You could also zero the entire buffer before calling inputText. But this would only disguise the real problem. Which is that you need to use a null-terminated pointer/slice type or specify a slice with a length equal to the offset of the first null character.

[mifopen]

Thank you @hazeycode! This makes a lot of sense. I agree that it is not obvious how thin should Zig binding libs be in general. I will close the issue then.

[Pyrolistical]

There are 2 different bugs interacting here.

1. var buf: [128]u8 = undefined; is being incorrectly zeroed. I think it is caused by Debug mode fails to set undefined memory to 0xaa for const/static var ziglang/zig#19538
2. zgui.inputText incorrectly takes in a buf: []u8, when it should be a buf: [:0]u8.

imgui uses the total buf length to know what is the maximum text size, but uses a null sentinel to know what is the current text length. This is exactly what [:0]u8 means.

I'll get a PR up to fix the later

[michaelbartnett]

@Pyrolistical I don't think changing the type of buf from []u8 to [:0]u8 accomplishes anything. The core problem is what @hazeycode pointed out: the buf being passed is storage, not the subspan of the storage that's considered to be the printable part of the string, so you need to sliceTo on the buffer afterward to get the printable slice.

You can reproduce the exact same behavior that @mifopen described with the same setup. All this does is ensure that at the end of the storage there's a null terminator. Maybe that's desirable, but it also means that code which passes the result of ArrayList(u8).allocatedSlice() as the buffer parameter needs to cast to a sentinel-terminated slice, which may trip safety checks unless you take care to write a 0 at the end of storage every time after you possibly grow the arraylist storage.

Run this test:

var input_buffer: [8:0]u8 = .{0} ** 8;

fn draw() void {
    const changed = zgui.inputText("Input text", .{
        .buf = &self.input_buffer,
        .flags = .{ .enter_returns_true = true },
    });
    if (changed) {
        std.log.info("Input raw array: {s}", .{self.input_buffer});
        std.log.info("Input sliced-to: {s}", .{std.mem.sliceTo(&self.input_buffer, 0)});
    }
}

Then use shift+right to highlight the "678" then type "0", and then hit enter. This will print:

info: Input raw array: 123450?8
info: Input sliced-to: 123450

Maybe it makes sense to add a more ergonomic API that takes an arraylist? This is what I had in my code:

fn inputTextArrayList(
    label: [:0]const u8,
    buffer: *std.ArrayList(u8),
    flags: zgui.InputTextFlags,
) !bool {
    const add_nullterm = buffer.items.len == 0 or buffer.items[buffer.items.len - 1] != 0;

    if (add_nullterm) {
        try buffer.append(0);
    }

    var use_flags = flags;
    use_flags.callback_resize = true;

    const InputResizeCallback = struct {
        fn callback(data: *zgui.InputTextCallbackData) i32 {
            std.debug.assert(data.event_flag.callback_resize);
            var b: *std.ArrayList(u8) = @alignCast(@ptrCast(data.user_data));

            b.ensureTotalCapacity(@intCast(data.buf_size)) catch @panic("OOM");
            b.items.len = @intCast(data.buf_text_len + 1);
            data.buf = b.items.ptr;
            data.buf_size = @intCast(b.capacity);

            return 0;
        }
    };

    const changed = zgui.inputText(label, .{
        .buf = buffer.allocatedSlice(), // doesn't compile now since this doesn't return a sentinel slice
        .flags = use_flags,
        .user_data = buffer,
        .callback = InputResizeCallback.callback,
    });

    if (add_nullterm) {
        std.debug.assert(buffer.items[buffer.items.len - 1] == 0);
        _ = buffer.pop();
    }
    return changed;
}

Or maybe a wrapper can return a struct with both a changed field and the new slice of strlen.

[Pyrolistical]

[:0]u8 is absolutely needed. A buffer overflow will occur otherwise.

See for yourself. We can restore old []u8 code by adding this to gui.zig:

pub fn inputText2(label: [:0]const u8, args: struct {
    buf: []u8,
    flags: InputTextFlags = .{},
    callback: ?InputTextCallback = null,
    user_data: ?*anyopaque = null,
}) bool {
    return zguiInputText(
        label,
        args.buf.ptr,
        args.buf.len,
        args.flags,
        if (args.callback) |cb| cb else null,
        args.user_data,
    );
}

Then apply this patch:

iff --git a/samples/gui_test_wgpu/src/gui_test_wgpu.zig b/samples/gui_test_wgpu/src/gui_test_wgpu.zig
index b5161488..61f51d1f 100644
--- a/samples/gui_test_wgpu/src/gui_test_wgpu.zig
+++ b/samples/gui_test_wgpu/src/gui_test_wgpu.zig
@@ -483,7 +483,8 @@ fn update(demo: *DemoState) !void {

         if (zgui.collapsingHeader("Widgets: Input with Keyboard", .{})) {
             const static = struct {
-                var input_text_buf = [_:0]u8{0} ** 4;
+                var input_text_buf = [_:0]u8{ 'h', 'e', 'l', 'l', 'o' };
+                var input_text_buf2 = [_]u8{ 'h', 'e', 'l', 'l', 'o' };
                 var input_text_multiline_buf = [_:0]u8{0} ** 4;
                 var input_text_with_hint_buf = [_:0]u8{0} ** 4;
                 var v1: f32 = 0;
@@ -501,6 +502,7 @@ fn update(demo: *DemoState) !void {
             zgui.separatorText("static input text");
             _ = zgui.inputText("Input text", .{ .buf = static.input_text_buf[0..] });
             _ = zgui.text("length of Input text {}", .{std.mem.len(@as([*:0]u8, static.input_text_buf[0..]))});
+            _ = zgui.inputText2("Input text2", .{ .buf = static.input_text_buf2[0..] });

             _ = zgui.inputTextMultiline("Input text multiline", .{ .buf = static.input_text_multiline_buf[0..] });
             _ = zgui.text("length of Input text multiline {}", .{std.mem.len(@as([*:0]u8, static.input_text_multiline_buf[0..]))});

Run zig build gui_test_wgpu-run and see the buffer overflow.

[copygirl]

@Pyrolistical This only happens if you feed it an incorrect buffer that doesn't have the terminating NUL character. Any code that modifies the buffer should ensure that after the text typed, a NUL character is present. You're creating the buffer by hand, not ensuring it contains NUL, and then "complaining" that it reads past the buffer.

If you want your input to be able to contain 256 characters, you create a buffer with length of 257. Then you can modify that as needed, and a NUL should always be present after the valid text. You can't cram 257 valid characters into a buffer of length 257, which needs to contain the terminating NUL.

I haven't used zgui, but I assume the input text is supposed to allow you to modify the buffer by typing into it? Why would you especially craft a [_:0]u8{ 'h', 'e', 'l', 'l', 'o' } when you'd likely want to be able to type more than 5 characters?

[michaelbartnett]

++ @copygirl

The example code was incorrect.

input_text_buf2 should be declared as a [_:0]u8 but inputText should still accept []u8, and it will work great because the [:0]u8 will coerce to []u8 with len + 1. That will solve the problem in that example.

And to cite @copygirl again, using a buffer that's exactly as long as your default input is abnormal usage of imgui. You normally over-provision if you're using fixed input storage.

If we wanna protect against overflow in cases where you forget to manually add a terminator or initially declare your storage as a sentinel array, the better move imo is to just write a 0 before calling the extern:

pub fn inputText2(label: [:0]const u8, args: struct {
    buf: []u8,
    flags: InputTextFlags = .{},
    callback: ?InputTextCallback = null,
    user_data: ?*anyopaque = null,
}) bool {
    buf[buf.len - 1] = 0; // <-------- prevent overflow
    return zguiInputText(
        label,
        args.buf.ptr,
        args.buf.len,
        args.flags,
        if (args.callback) |cb| cb else null,
        args.user_data,
    );
}

The end user will probably see their input is cutoff unexpectedly and this can be a clue to them that they need to use a sentinel array.

[Pyrolistical]

Any code that modifies the buffer should ensure that after the text typed, a NUL character is present.

If you want your input to be able to contain 256 characters, you create a buffer with length of 257. Then you can modify that as needed, and a NUL should always be present after the valid text.

Yes which is exactly what [:0]u8 means. It doesn't mean that null must be at len, a null could be present earlier for the true length of the string.

I still don't understand what problem is solved by allowing []u8.

The purpose of my example is to reveal of the bug when providing a filled buffer. Of course in real code the provided buffer would be much longer.

[michaelbartnett]

The problem is that it introduces footguns for people implementing the resize callback.

Previously you could pass your storage slice directly to the buf parameter (using an ArrayList here because it's common and useful for this):

if (arraylist.items.len == 0 or arraylist.items[arraylist.items.len - 1] != 0) {
    arraylist.append(0) catch @panic("OOM");
}
const slice = arraylist.allocatedSlice();
_ = zgui.inputText("InputLabel", .{
    .buf = @ptrCast(slice),
    .user_data = &arraylist,
    .flags = .{ .callback_resize = true },
    .callback = struct {
        fn resize(data: *zgui.InputTextCallbackData) i32 {
            var b: *std.ArrayList(u8) = @alignCast(@ptrCast(data.user_data));
            b.ensureTotalCapacity(@intCast(data.buf_size)) catch @panic("OOM");
            b.items.len = @intCast(data.buf_text_len);
            data.buf = b.items.ptr;
            data.buf_size = @intCast(b.capacity);
            return 0;
        }
    }.resize,
});
if (arraylist.items[arraylist.items.len - 1] == 0) {
    _ = arraylist.pop();
}

But now because of the sentinel terminated slice, you have to both ptrCast the slice, and also remember to take a subslice (len-1) so that the buf.len + 1 in zgui.inputText doesn't cause you to allocate on every keystroke and eventually OOM:

--- before.zig
+++ after.zig
if (arraylist.items.len == 0 or arraylist.items[arraylist.items.len - 1] != 0) {
    arraylist.append(0) catch @panic("OOM");
}
const slice = arraylist.allocatedSlice();
_ = zgui.inputText("InputLabel", .{
-     .buf = slice,
+     .buf = @ptrCast(slice[0 .. slice.len - 1]),
    .user_data = &arraylist,
    .flags = .{ .callback_resize = true },
    .callback = struct {
        fn resize(data: *zgui.InputTextCallbackData) i32 {
            var b: *std.ArrayList(u8) = @alignCast(@ptrCast(data.user_data));
            b.ensureTotalCapacity(@intCast(data.buf_size)) catch @panic("OOM");
            b.items.len = @intCast(data.buf_text_len);
            data.buf = b.items.ptr;
            data.buf_size = @intCast(b.capacity);
            return 0;
        }
    }.resize,
});
if (arraylist.items[arraylist.items.len - 1] == 0) {
    _ = arraylist.pop();
}

If you just ptrCast the slice and don't adjust the len, you'll OOM after 40 or 50 keystrokes because each time you're adding +1 to the buf len, which is normally a signal in this callback that a resize is required.

Also, the version with the slice length adjustment behaves correctly, but it still does not guarantee a null terminator at the end of the arraylist's allocated buffer. It's not a problem currently, but if runtime safety checks are ever introduced which check for that null terminator during ptrCasts (or someone tries to use a helper function for casting) then this will likely trip that check and panic.

So in addition to remembering to reduce the slice len before passing it in, you also ought to add a null terminator at buf.len - 1 so you're not lying to ptrCast.

Maybe all that would be fine if we were getting a big benefit from this, but I don't think we get much from it. It doesn't solve the original problem reported here, and the issue of the buffer overrun you showed never manfiests so long as your initial input has a null terminator, and this is easy to do:

var empty_storage = [1]u8{0} ** 32;
var storage_with_initial_text = ("initial input" ++ [1]u8{0} ** 32).*;

Or even if you wanna be lazy, your storage can still just be an undefined sentinel array and it will still work:

var input_storage: [32:0]u8 = undefined;

The reason it still works is because when you pass the slice of your sentinel array to zgui.inputText the coercion from [:0]u8 to []u8 increases len by 1, and now you're cruising.

The contract of the ImGui::InputText API is that you provide the current buffer with a null terminator somewhere but not necessarily at the end of the buffer. As long as you do that, imgui will not overrun the buffer. The [:0]u8 parameter type is expressing the wrong thing, and the right thing--having a null terminator somewhere in the input buffer but not necessarily at the end--is not expressable in Zig.

[Pyrolistical]

From the zig docs

The syntax [:x]T is a slice which has a runtime-known length and also guarantees a sentinel value at the element indexed by the length. The type does not guarantee that there are no sentinel elements before that.

So it's not perfect but also not wrong

[michaelbartnett]

It's not broken or whatever, for sure, but the api contract of ImGui::InputText doesn't care about having a null terminator at the end of the input buffer. So the constraint on the parameter has limited utility.

And limited utility would be fine, except it also introduces this footgun with handling input buffer resizes. Feels like a net negative to me.