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.

Sign up for GitHub

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

implement a ubsan runtime for better error messages #22488

Merged
merged 22 commits into from
Feb 26, 2025
Merged

implement a ubsan runtime for better error messages #22488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
5e0073c
ubsan: add a basic runtime
Rexicon226 Dec 17, 2024
eef8d4f
ubsan: switch to using `std.builtin.panicExtra` to log errors
Rexicon226 Dec 17, 2024
c27b797
Compilation: use the minimal runtime in `ReleaseSafe`
Rexicon226 Dec 18, 2024
babee5f
ubsan: implement some more checks
Rexicon226 Dec 25, 2024
95720f0
move libubsan to `lib/` and integrate it into `-fubsan-rt`
Rexicon226 Dec 26, 2024
fc77678
mem: add `@branchHint` to `indexOfSentinel`
Rexicon226 Dec 26, 2024
590c613
ubsan: implement more checks
Rexicon226 Dec 26, 2024
658fba9
ubsan: extend `ptr` before adding to avoid overflow
Rexicon226 Dec 26, 2024
50b9556
ubsan: clean-up and remove the unused handlers
Rexicon226 Dec 26, 2024
a468929
ubsan: resolve the last of the TODOs
Rexicon226 Dec 26, 2024
2d4574a
Compilation: always import ubsan if a ZCU exists
Rexicon226 Dec 26, 2024
1417847
main: add `-f{no-}ubsan-rt` to the usage text
Rexicon226 Dec 26, 2024
d669b95
ubsan: clean-up a bit more
Rexicon226 Dec 27, 2024
9432a9b
build: add `bundle_ubsan_rt`
Rexicon226 Dec 27, 2024
9311784
Compilation: correct when to include ubsan
Rexicon226 Jan 1, 2025
35b9db3
correct some bugs
Rexicon226 Jan 14, 2025
44d3b5a
build: add comments explaining why we disable ubsan
Rexicon226 Feb 3, 2025
d4413e3
ubsan: avoid depending on `@returnAddress` combined with `inline`
andrewrk Feb 24, 2025
e18c7f9
ubsan: don't create ubsan in every static lib by default
andrewrk Feb 24, 2025
faf256e
std.mem.indexOfSentinel: don't ask the OS the page size
andrewrk Feb 24, 2025
2447b87
std.heap.page_size_min: relax freestanding restriction
andrewrk Feb 24, 2025
ca83f52
ubsan: update wording
Rexicon226 Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/std/Build/Step/Compile.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ compress_debug_sections: enum { none, zlib, zstd } = .none,
verbose_link: bool,
verbose_cc: bool,
bundle_compiler_rt: ?bool = null,
bundle_ubsan_rt: ?bool = null,
rdynamic: bool,
import_memory: bool = false,
export_memory: bool = false,
Expand Down Expand Up @@ -1563,6 +1564,7 @@ fn getZigArgs(compile: *Compile, fuzz: bool) ![][]const u8 {
}

try addFlag(&zig_args, "compiler-rt", compile.bundle_compiler_rt);
try addFlag(&zig_args, "ubsan-rt", compile.bundle_ubsan_rt);
try addFlag(&zig_args, "dll-export-fns", compile.dll_export_fns);
if (compile.rdynamic) {
try zig_args.append("-rdynamic");
Expand Down
10 changes: 5 additions & 5 deletions lib/std/heap.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,8 @@ pub var next_mmap_addr_hint: ?[*]align(page_size_min) u8 = null;
///
/// On many systems, the actual page size can only be determined at runtime
/// with `pageSize`.
pub const page_size_min: usize = std.options.page_size_min orelse (page_size_min_default orelse if (builtin.os.tag == .freestanding or builtin.os.tag == .other)
@compileError("freestanding/other page_size_min must provided with std.options.page_size_min")
else
@compileError(@tagName(builtin.cpu.arch) ++ "-" ++ @tagName(builtin.os.tag) ++ " has unknown page_size_min; populate std.options.page_size_min"));
pub const page_size_min: usize = std.options.page_size_min orelse page_size_min_default orelse
@compileError(@tagName(builtin.cpu.arch) ++ "-" ++ @tagName(builtin.os.tag) ++ " has unknown page_size_min; populate std.options.page_size_min");

/// comptime-known maximum page size of the target.
///
Expand Down Expand Up @@ -831,8 +829,10 @@ const page_size_min_default: ?usize = switch (builtin.os.tag) {
.xtensa => 4 << 10,
else => null,
},
.freestanding => switch (builtin.cpu.arch) {
.freestanding, .other => switch (builtin.cpu.arch) {
.wasm32, .wasm64 => 64 << 10,
.x86, .x86_64 => 4 << 10,
.aarch64, .aarch64_be => 4 << 10,
else => null,
},
Comment on lines -834 to 837
Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on your thinking here? freestanding means there is no OS so there is no guarantee that the concept of a "page" is even meaningful. For all the standard library knows, paging might not be set up at all. So I don't see how this change is correct.

For other there's a similar problem: The implication is that there is an OS (as opposed to freestanding), but we still don't know what that OS is, so we can't make any valid claims about its minimum page size.

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that the page size is a CPU architecture feature - the OS has a limited set of choices available based on the CPU architecture. The OS can't pick any arbitrary value for page size. Do you have reason to believe this is not the case?

Copy link
Member

Choose a reason for hiding this comment

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

The interpretation of "minimum and maximum page size" that we're using here is basically "the minimum and maximum granularity that mmap can be configured to operate at" (e.g. at kernel build time). We chose this interpretation specifically because if we just went by the CPU architecture's minimum/maximum, we would get into absurd territory quickly; most CPU architectures support page sizes that are completely impractical as the normal page size of a general-purpose OS, and so these page sizes are either not supported at all by OSs, or only supported with special opt-in flags (e.g. MAP_HUGE*).

Obviously this interpretation is not very meaningful in freestanding because for all we know there's no mmap syscall/function at all. But that's fine, because for all we know there may not be any paging at all anyway. And that implies that the standard library must not exploit knowledge about the supposed safety of overlong reads within a page, for example. So for freestanding specifically, it just doesn't really make sense to speak of page size. If there actually is one in effect, the programmer needs to tell us about it. Otherwise we're making a potentially invalid assumption.

For other, the situation is similar. We know there's an OS, but not much else; there's a good chance that it does have mmap of some description, but it could certainly also be an OS without paging. We just don't know. (This makes me think that there might not be as much utility in the other OS tag as was originally thought, but I digress.)

(I suppose you could also argue that it's possible for an OS to have mmap operate at a granularity that has nothing to do with the page sizes supported by the CPU architecture. But I don't know why you would do something that silly, and I don't think anyone has, so this probably need not be a consideration.)

Copy link
Member

Choose a reason for hiding this comment

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

(I suppose you could also argue that it's possible for an OS to have mmap operate at a granularity that has nothing to do with the page sizes supported by the CPU architecture. But I don't know why you would do something that silly, and I don't think anyone has, so this probably need not be a consideration.)

I'm arguing precisely the opposite: it's not possible for an OS to have mmap operate at a granularity different than the page sizes supported by the CPU architecture, which means that page_size_min being 4096 on x86_64 is correct, including on freestanding.

Copy link
Member

@andrewrk andrewrk Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's make room for this PR to land, and then hash this out separately. std.mem.indexOfPos is an important function, so we need to give it the appropriate amount of engineering effort it deserves. That means even though I'm tempted to simply say, "this violates pointer provenance, fuck it," I still need to consider whether a page-aware implementation should be considered. It absolutely needs to work on freestanding by default however.

@tiehuis, if available, should be involved in that discussion.

Copy link
Member

Choose a reason for hiding this comment

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

I'm arguing precisely the opposite: it's not possible for an OS to have mmap operate at a granularity different than the page sizes supported by the CPU architecture, which means that page_size_min being 4096 on x86_64 is correct, including on freestanding.

Just to be clear, what I was getting at with that paragraph is that you can do some incredibly silly stuff with page protection and instruction emulation/resumption at the supervisor level to make it appear to userspace as if the machine has page sizes that don't look anything like the architectural page sizes. It'd be very slow and very stupid, which is why I also said it probably shouldn't be a real consideration.

Let's make room for this PR to land, and then hash this out separately.

👍 But just out of curiosity, why were these changes needed in this PR? That's not clear from the commit messages.

It absolutely needs to work on freestanding by default however.

Agree; I just think the answer here is to avoid the page-size-dependent code paths altogether in the freestanding case.

Copy link
Member

Choose a reason for hiding this comment

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

👍 But just out of curiosity, why were these changes needed in this PR? That's not clear from the commit messages.

Ah, sorry I thought you saw - ubsan_rt depends on std.mem.indexOfSentinel, which was calling std.heap.pageSize(). When that function was implemented, it was accessing a comptime value; in the runtime page size changeset, it was carelessly updated to be a runtime-known value, regressing the function on freestanding/other targets.

https://github.com/ziglang/zig/actions/runs/13497343926/job/37707452929

Copy link
Member

Choose a reason for hiding this comment

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

👍

When that function was implemented, it was accessing a comptime value; in the runtime page size changeset, it was carelessly updated to be a runtime-known value, regressing the function on freestanding/other targets.

Wait, was there ever a fully comptime pageSize() function? I thought it was just a page_size constant before. FWIW, it's intentional that pageSize() is potentially runtime: The actual page size in effect can be different (and often is, e.g. on aarch64) from the page_size_min/page_size_max constants. The latter only tell you the bounds of possible values for pageSize().

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, by "that function", I meant std.mem.indexOfSentinel.

else => null,
Expand Down
7 changes: 4 additions & 3 deletions lib/std/mem.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1098,12 +1098,12 @@ pub fn indexOfSentinel(comptime T: type, comptime sentinel: T, p: [*:sentinel]co
// as we don't read into a new page. This should be the case for most architectures
// which use paged memory, however should be confirmed before adding a new arch below.
.aarch64, .x86, .x86_64 => if (std.simd.suggestVectorLength(T)) |block_len| {
const page_size = std.heap.pageSize();
const page_size = std.heap.page_size_min;
const block_size = @sizeOf(T) * block_len;
const Block = @Vector(block_len, T);
const mask: Block = @splat(sentinel);

comptime assert(std.heap.page_size_max % @sizeOf(Block) == 0);
comptime assert(std.heap.page_size_min % @sizeOf(Block) == 0);
assert(page_size % @sizeOf(Block) == 0);

// First block may be unaligned
Expand All @@ -1119,6 +1119,7 @@ pub fn indexOfSentinel(comptime T: type, comptime sentinel: T, p: [*:sentinel]co

i += @divExact(std.mem.alignForward(usize, start_addr, block_size) - start_addr, @sizeOf(T));
} else {
@branchHint(.unlikely);
// Would read over a page boundary. Per-byte at a time until aligned or found.
// 0.39% chance this branch is taken for 4K pages at 16b block length.
//
Expand Down Expand Up @@ -1152,7 +1153,7 @@ pub fn indexOfSentinel(comptime T: type, comptime sentinel: T, p: [*:sentinel]co
test "indexOfSentinel vector paths" {
const Types = [_]type{ u8, u16, u32, u64 };
const allocator = std.testing.allocator;
const page_size = std.heap.pageSize();
const page_size = std.heap.page_size_min;

inline for (Types) |T| {
const block_len = std.simd.suggestVectorLength(T) orelse continue;
Expand Down
Loading
Loading