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

UEFI.pool_allocator.resize() Unreachable Code when length and capacity match on an ArrayList #21446

Closed
dje4321 opened this issue Sep 18, 2024 · 1 comment · Fixed by #21456
Closed
Labels
bug Observed behavior contradicts documented or intended behavior os-uefi standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@dje4321
Copy link

dje4321 commented Sep 18, 2024

Zig Version

0.13.0

Steps to Reproduce and Observed Behavior

Working on a UEFI zig project and keep hitting a random panic when trying to get an ownedSlice from an arraylist. Eventually narrowed it down to only be occurring when capacity and length match on an array list. Manually incrementing the capacity by 1 causes the panic branch to be missed.

The following code will reliably hit the panic branch on my machine. I know its not perfect, pretty, or correct however, it does its job just fine. It could probably be cleaned up by a fair bit but this is what I was able to quickly throw together to get around the compiler optimizing away the code.

    const list = std.ArrayList(*u8);
    var u8_list = list.init(std.os.uefi.pool_allocator);
    u8_list.append(@constCast(@as(*u8, (@ptrFromInt(8))))) catch @panic(":(");
    u8_list.append(@constCast(@as(*u8, (@ptrFromInt(1))))) catch @panic(":(");
    u8_list.append(@constCast(@as(*u8, (@ptrFromInt(2))))) catch @panic(":(");
    u8_list.append(@constCast(@as(*u8, (@ptrFromInt(3))))) catch @panic(":(");
    u8_list.append(@constCast(@as(*u8, (@ptrFromInt(4))))) catch @panic(":(");
    u8_list.append(@constCast(@as(*u8, (@ptrFromInt(5))))) catch @panic(":(");
    u8_list.append(@constCast(@as(*u8, (@ptrFromInt(6))))) catch @panic(":(");
    u8_list.append(@constCast(@as(*u8, (@ptrFromInt(7))))) catch @panic(":(");
    // Len & Cap are now both 8

    const s = u8_list.toOwnedSlice() catch @panic("Failure");
    for (s) |t| {
        _ = t;
    }
    @panic("Should have hit unreachable code");

The code panics with the msg reached unreachable code. Digging around the code further revealed it to be caused by the resize method being performed in the toOwnedSlice() call allocator.resize(old_memory, self.items.len)

https://ziglang.org/documentation/master/std/#src/std/array_list.zig

Currently dont have any way to debug zig UEFI applications so this is as far as I was able to take this atm.

Expected Behavior

Standard library should not panic

@dje4321 dje4321 added the bug Observed behavior contradicts documented or intended behavior label Sep 18, 2024
@truemedian
Copy link
Contributor

This bug probably exhibits itself in a lot of different scenarios. pool_allocator and raw_pool_allocator pass log2_old_ptr_align to mem.alignAllocLen, which expects len_align (which would be 2^log2_old_ptr_align).

Further: neither of these resize functions has any reason to call mem.alignAllocLen because it immediately throws away the result.

truemedian added a commit to truemedian/zig that referenced this issue Sep 19, 2024
Fixes ziglang#21446

Both UefiPoolAllocator and UefiRawPoolAllocator were
passing the value of `log2_ptr_align` directly to
`mem.alignAllocLen` which expects a alignment value.

Both of these calls to `mem.alignAllocLen` are pointless
and the result of the alignment both always true, and
was thrown away anyway.

I have removed these calls entirely.
@Vexu Vexu added standard library This issue involves writing Zig code for the standard library. os-uefi labels Sep 20, 2024
@Vexu Vexu added this to the 0.14.0 milestone Sep 20, 2024
DivergentClouds pushed a commit to DivergentClouds/zig that referenced this issue Sep 24, 2024
Fixes ziglang#21446

Both UefiPoolAllocator and UefiRawPoolAllocator were
passing the value of `log2_ptr_align` directly to
`mem.alignAllocLen` which expects a alignment value.

Both of these calls to `mem.alignAllocLen` are pointless
and the result of the alignment both always true, and
was thrown away anyway.

I have removed these calls entirely.
richerfu pushed a commit to richerfu/zig that referenced this issue Oct 28, 2024
Fixes ziglang#21446

Both UefiPoolAllocator and UefiRawPoolAllocator were
passing the value of `log2_ptr_align` directly to
`mem.alignAllocLen` which expects a alignment value.

Both of these calls to `mem.alignAllocLen` are pointless
and the result of the alignment both always true, and
was thrown away anyway.

I have removed these calls entirely.
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 os-uefi 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.

3 participants