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

IoUring.copy_cqes_ready panics on @memcopy #19451

Closed
krichprollsch opened this issue Mar 27, 2024 · 2 comments · Fixed by #19501
Closed

IoUring.copy_cqes_ready panics on @memcopy #19451

krichprollsch opened this issue Mar 27, 2024 · 2 comments · Fixed by #19501
Labels
bug Observed behavior contradicts documented or intended behavior os-linux regression It worked in a previous version of Zig, but stopped working. standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@krichprollsch
Copy link

Zig Version

0.12.0-dev.3439+31a7f22b8

Steps to Reproduce and Observed Behavior

Running this test case, the function IoUring.copy_cqes_ready panics with the error @memcpy arguments have non-equal lengths.

const std = @import("std");
const linux = std.os.linux;

test {
    var ring = try linux.IoUring.init(2, 0);
    defer ring.deinit();

    const exp = 4;
    var i: u32 = 0;
    while (i < exp) {
        const sqe = try ring.get_sqe();
        sqe.prep_timeout(&.{ .tv_sec = 0, .tv_nsec = 10000 }, 0, 0);
        _ = try ring.submit();
        i += 1;
    }

    var count: u32 = 0;
    while (true) {
        var cqes: [256]linux.io_uring_cqe = undefined;
        const c = try ring.copy_cqes(&cqes, 0);

        count += c;
        if (count == exp) break;
    }
}
$ zig test src/main.zig
Test [1/1] main.test_0... thread 640540 panic: @memcpy arguments have non-equal lengths
/usr/local/zig/lib/std/os/linux/IoUring.zig:289:45: 0x103dada in copy_cqes_ready (test)
        @memcpy(cqes[0..count], self.cq.cqes[head..tail]);
                                            ^
/usr/local/zig/lib/std/os/linux/IoUring.zig:272:39: 0x103d66b in copy_cqes (test)
    const count = self.copy_cqes_ready(cqes);
                                      ^
../src/main.zig:22:37: 0x103e385 in test_0 (test)
        const c = try ring.copy_cqes(&cqes, 0);
                                    ^
/usr/local/zig/lib/compiler/test_runner.zig:158:25: 0x107868c in mainTerminal (test)
        if (test_fn.func()) |_| {
                        ^
/usr/local/zig/lib/compiler/test_runner.zig:35:28: 0x1040adb in main (test)
        return mainTerminal();
                           ^
/usr/local/zig/lib/std/start.zig:501:22: 0x103e9b9 in posixCallMainAndExit (test)
            root.main();
                     ^
/usr/local/zig/lib/std/start.zig:253:5: 0x103e521 in _start (test)
    asm volatile (switch (native_arch) {
    ^
???:?:?: 0x0 in ??? (???)
error: the following test command crashed:
../zig-cache/o/022070b5f4a71d4acb1438f3f0b3903b/test

Expected Behavior

The test should pass.

@krichprollsch krichprollsch added the bug Observed behavior contradicts documented or intended behavior label Mar 27, 2024
@krichprollsch
Copy link
Author

I think the issue relates to #18757
I'm not sure, but it looks like it happens when head == tail.
If I apply this patch, the test passes.

diff --git a/lib/std/os/linux/IoUring.zig b/lib/std/os/linux/IoUring.zig
index 0e3c4ce338..2c2945a558 100644
--- a/lib/std/os/linux/IoUring.zig
+++ b/lib/std/os/linux/IoUring.zig
@@ -284,7 +284,7 @@ fn copy_cqes_ready(self: *IoUring, cqes: []linux.io_uring_cqe) u32 {
     const head = self.cq.head.* & self.cq.mask;
     const tail = (self.cq.head.* +% count) & self.cq.mask;
 
-    if (head <= tail) {
+    if (head < tail) {
         // head behind tail -> no wrapping
         @memcpy(cqes[0..count], self.cq.cqes[head..tail]);
     } else {

@Vexu Vexu added standard library This issue involves writing Zig code for the standard library. os-linux labels Mar 27, 2024
@Vexu Vexu added this to the 0.13.0 milestone Mar 27, 2024
@ianic
Copy link
Contributor

ianic commented Mar 31, 2024

#18757 is regression, it does not handle head == tail correctly!

With the example above IoUring gets initialized with self.cq.cqes of len 4.
When we enter copy_cqes_ready

self.cq.cqes = 4
self.cq.mask = 3

ready = 4
count = 4
head = 0
tail = 0 <= wrapping

And then head <= tail assumes that there is no wrapping and calls memcpy with two buffers one of size 4 and another of size 0.

Changing <= to < will fix logic.
But I'll suggest that we revert #18757 and stick with the previous implementation.

This one is, in my opinion, much harder to follow. Original liburing has the same logic. If we are compiled with compiler_rt memcpy we have same loop but now with some more branching.

ianic added a commit to ianic/zig that referenced this issue Mar 31, 2024
@andrewrk andrewrk modified the milestones: 0.13.0, 0.12.0 Apr 1, 2024
@andrewrk andrewrk added the regression It worked in a previous version of Zig, but stopped working. label Apr 1, 2024
Rexicon226 pushed a commit to Rexicon226/zig that referenced this issue Apr 3, 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 os-linux regression It worked in a previous version of Zig, but stopped working. 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.

4 participants