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

disable fuzzer on 32 bit linux #23008

Closed

Conversation

bradobro
Copy link

Addresses #22523 by:

  1. only allowing fuzz testing on 64-bit platforms
  2. add a test and @truncate, NOP on 64-bit platforms, which still allows building on 32-bit platforms

This follows a commit that disallows fuzzing on 32-bit platforms, but it still has to build given the current build logic.
// More specifically, posix.mmap()'s second parameter, `length: usize`,
// must be the same bit-size as std.fs.getEndPos()'s return value, which is `u64`.
// Affects or affected by issues #5185, #22523, and #22464.
64 => {},
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows fuzz testing only on 64-bit platforms, but it's not enough to fix the build runner on 32-bit platforms.

log.err("coverage file '{}' is too large to mmap", .{coverage_file_path});
return error.OutOfMemory;
}
const file_usize: usize = @truncate(file_size);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This @truncate is NOP for 64-bit platforms but necessary for the build to work on 32-bit platforms, even though we don't allow running fuzz tests of 32-bit platforms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why? This code shouldn't be compiled for 32-bit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but where is the build logic to exclude it on 32-bit?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines that I pointed out earlier are a chokepoint, so the patch there will make all of WebServer.zig dead code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake--a made a goof while testing.

I've removed these changes. Would you like a new PR omitting the changeset introducing changes to WebServer.zig?

@@ -628,9 +628,17 @@ fn prepareTables(
return error.AlreadyReported;
};

// Account for bit-size mismatch between fs.getEndPos() and posix.mmap() ideas about file length
if (file_size >= std.math.maxInt(usize)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is not necessary so long as we prohibit fuzz testing on 32-bit platforms.

However, it shows one possible way of safely trying it out on those platforms. The actual test could be affected, for libc targets, by -D_FILE_OFFSET_BITS. Raspbian 32-bit man 2 stat notes:

EOVERFLOW
pathname or fd refers to a file whose size, inode number, or number of blocks cannot be represented
in, respectively, the types off_t, ino_t, or blkcnt_t. This error can occur when, for example, an
application compiled on a 32-bit platform without -D_FILE_OFFSET_BITS=64 calls stat() on a file
whose size exceeds (1<<31)-1 bytes.

@bradobro
Copy link
Author

@andrewrk, here's a draft PR for #22523. See code comments in this PR.

@bradobro bradobro closed this Feb 26, 2025
@bradobro bradobro deleted the 22523-disable-fuzzer-on-32-bit-linux branch February 26, 2025 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants