-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Generate checks for all forms of UB in Debug and ReleaseSafe modes #2301
Comments
Interestingly, this may be related to #130. One current area of unsafety is when you use struct embedding and |
any improvement? |
IMHO the important thing to get right in regards to safety is memory safety. Existing publications on this topic often make a distinction between spatial and temporal memory safety, e.g. [1]:
As it stands currently, I think Zig offers neither spatial nor temporal memory safety. Haven't really used the language much yet but it seems to me that |
Curious about what could be the strategy to make Zig safe against use-after-free of a local function stack variable? Do you think it's possible through static analysis or by crashing at runtime in Debug and ReleaseSafe modes? |
Latest relevant blog post summary by scattered thoughts:
My thoughts I realized this after I tried to understand free (called drop in Rust). Since we would need to require 1.lifetime annotations on functions and types as 2.parseable comments, 3.no union(type compression): Is it feasible to separate a program into memory checked and unchecked parts? 5: Stackoverflows can be fixed via static analysis of recursion and worst case stack over-approximation, which would even give a stronger guarantee than Rust (no stackoverflows). This information can be used for optimization and thus should remain closely tight to the compiler. However, this does not include pointer access to stack. 6: UUM checker, but I believe we can restrict or optimize the analysis due to usingnamespace making usage of symbols explicit. This only leaves potential linker hacks to initialize variables as potential pitifal #9512 and external symbols in linked libraries/C code. Probably the same strategy as 7 (emitting symbols for external analysis) should be done. 7: To decide this (in a simplified model), one either needs graph analysis or deduce all usage combinations. APPENDUM |
Sketch of borrow checking (BC). I dont know yet, if and where I did flawed assumptions. Will probably sleep a few nights overthinking it. Based on Nikos presentation on Polonius. (I think) The decision for or against datafrog is motivated by
On applying BC (we simplify allocation here):
The BC analysis then must abstract over loops to generate rules and figure out, if any function evaluation resolves to satisfying or unsatisfying lifetime constraints (1.order of how memory is allocated or deallocated, 2.pointer validity with lifetime rules). The difficult part is 2: Pointer allow arbitrary indirection complexity and looking up the address the pointer points to crushes cache-locality relatively fast, because we must track the possible indirections over the whole lifetime of the program with annotating and resolving the lifetime rules. BC Fundamental rule
BC principles
var x: u16 = 100; //MS assume base address 0x400, len 2 (byte)
// ignore for now that p1 and p2 (and x) are not live (dead variable optimisation)
var p1: *const u16 = &x; //Rust: let p1 = &x //Loan1(L1): p1 borrows x shared, so x can not be written unless p1 goes of out of scope
var p2: *u16 = &x; //Rust: let p1 = mut &x //Loan2(L2): p2 borrows x mutable, so only p2 can write x. x can be used as usual.
var x: u16 = 22;
var p1: *const u16 = &x; //L1: p1 borrows x shared
x += 1; //error: x is modified, but was borrowed shared => lookup (what subparts) are shared in which mode
print("y: {}", .{p1.*}); //make sure p1 lives (see below)
var x: u8 = 1;
//x is dead here, because it is reassigned before usage
x = 2;
//x is live here, because there is no reassignment before usage
//common static analysis method: Live Variable Analysis (traverse CFA backwarsd)
print("x: {}", .{x});
const Foo = struct { x: u8 = 1, bar: u8 = 2 };
//TODO: make it properly
var foo:Foo = Foo{};
var x: = &foo.bar; //L1: x borrows foo mutable
var y = &foo; //L1: error, can not borrow foo[0,bar.end] => foo[bar.start,bar.end] mutable borrowed
//y gets used => loans L1 and L2 live
print("z: {}", .{y.*}); APPENDUM |
Since Rust borrow checker does not work for self referential types, I doubt if BC of this type is feasible? For example, is the following code disallowed? const Foo = struct { next: ?*Foo, };
var foo: Foo = .{ .next = null };
foo.next = &foo;// error, foo is already borrowed mutable? |
This is what Rust has Pin for. So in theory this should be possible, but in practice one would need to compute a compile-time derivable memory layout over the recursive structures. This works fine for lists, smaller/fixed-layout trees and simple stuff, but likely does not work anymore for more complex structures. (The idea is that you can only set null-pointer to non-null and non-null pointer must not be modified until you clean up recursively the structure.) By the way, your simplified example is currently broken on master: const std = @import("std");
const print = std.debug.print;
pub fn main() void {
const Foo = struct { t1: u8, next: ?*Foo }; //without type t1 zls or zig.vim immediately complains
var foo: Foo = .{ .t1 = 1, .next = null };
foo.next = &foo;
} |
Another interesting approach is Wuffs, which can be significantly fast. They can only do this, because they require that heap memory owned by pointers does not get fragmented. So programmers must manage pointer offsets to each heap structure themself. Hence one never gets potential pointer soup (pointers pointing to subparts and pointing to other things), which Rust resolves with (the costly) pointer lifetime checking. So memory handling is relative static and simple, because no subpointers or substructures to same memory region can be created. Also loop invariants etc must all be annotated, so one does basically proof all invariants manually and gets only the compiler confirmation. So overall this should be fine for anything simpler in terms of memory layout. |
@matu3ba So instead, in zig, except some really simple cases such as returning pointer to a stack-allocated variable, I think we should keep the language simple (I don't want to fight with BC even in zig hahaha, and we don't have "unsafe zig") and just do the safety checks at runtime (Debug and ReleaseSafe modes only of course), raise error immediately when a memory access violation happens. This makes the problem much easier to spot than in c or c++. For more information about the problem with Rust and ownership model, I recommend this article. (maybe we should consider moving to discussions?) |
@Hadron67 thanks for the great article and summary (though Pin does require unsafe due to move semantics in c++/Rust, which c and zig to my knowledge does not have). https://zigforum.org/ would be a candidate. |
How might future safety checks handle lifetime issues involving allocating containers like Example 1: reallocation invalidating a pointer const std = @import("std");
pub fn main() anyerror!void {
// Make an ArrayList with one element.
var list = std.ArrayList(i32).init(std.heap.c_allocator);
defer list.deinit();
try list.append(0);
// Take a pointer to that element.
const ptr = &list.items[0];
// Add some more elements, to force a reallocation.
var i: usize = 0;
while (i < 100) {
try list.append(0);
i += 1;
}
// BUG: Reallocation has invalidated our pointer, and dereferencing it is
// now a use-after-free.
std.debug.print("ptr.* = {}\n", .{ptr.*});
} Example 2: copying leading to a double free const std = @import("std");
pub fn main() anyerror!void {
// Make an ArrayList with one element.
var list1 = std.ArrayList(i32).init(std.heap.c_allocator);
defer list1.deinit();
try list1.append(0);
{
// Make another ArrayList with a shorter lifetime.
var list2 = std.ArrayList(i32).init(std.heap.c_allocator);
defer list2.deinit();
// Overwrite list2 with list1. This assignment is a shallow copy.
list2 = list1;
}
// BUG: list2.deinit() has freed list1's heap storage, so reading list1 is
// now a use-after-free, and list1.deinit() will be a double free.
std.debug.print("list1 = [{}]\n", .{list1.items[0]});
} |
@oconnor0 The GeneralPurposeAllocator catches these issues in most cases by avoiding reusing memory addresses for as long as possible. const std = @import("std");
pub fn main() anyerror!void {
var gpa = std.heap.GeneralPurposeAllocator(.{}){};
defer std.testing.expect(!gpa.deinit());
const alloc = &gpa.allocator;
// Make an ArrayList with one element.
var list = std.ArrayList(i32).init(alloc);
defer list.deinit();
try list.append(0);
// Take a pointer to that element.
const ptr = &list.items[0];
// Add some more elements, to force a reallocation.
var i: usize = 0;
while (i < 100) {
try list.append(0);
i += 1;
}
// BUG: Reallocation has invalidated our pointer, and dereferencing it is
// now a use-after-free.
std.debug.print("ptr.* = {}\n", .{ptr.*});
} Segmentation fault at address 0x7ff11876b000
/home/pfg/Dev/Node/temp/generated/c2eadb992fd9e98a212b0aa4ca835207/tmp/test.zig:24:42: 0x229e09 in main (test)
std.debug.print("ptr.* = {}\n", .{ptr.*});
^
/home/pfg/Apps/zig/zig-linux-x86_64-0.8.0-dev.2082+e9e91b4ed/lib/std/start.zig:420:37: 0x22256a in std.start.callMain (test)
const result = root.main() catch |err| {
^
/home/pfg/Apps/zig/zig-linux-x86_64-0.8.0-dev.2082+e9e91b4ed/lib/std/start.zig:362:12: 0x20653e in std.start.callMainWithArgs (test)
return @call(.{ .modifier = .always_inline }, callMain, .{});
^
/home/pfg/Apps/zig/zig-linux-x86_64-0.8.0-dev.2082+e9e91b4ed/lib/std/start.zig:316:17: 0x2057c9 in std.start.posixCallMainAndExit (test)
std.os.exit(@call(.{ .modifier = .always_inline }, callMainWithArgs, .{ argc, argv, envp }));
^
/home/pfg/Apps/zig/zig-linux-x86_64-0.8.0-dev.2082+e9e91b4ed/lib/std/start.zig:238:5: 0x2056f2 in std.start._start (test)
@call(.{ .modifier = .never_inline }, posixCallMainAndExit, .{});
^
fish: Job 1, 'zig run test.zig' terminated by signal SIGABRT (Abort) , const std = @import("std");
pub fn main() anyerror!void {
var gpa = std.heap.GeneralPurposeAllocator(.{}){};
defer std.testing.expect(!gpa.deinit());
const alloc = &gpa.allocator;
// Make an ArrayList with one element.
var list1 = std.ArrayList(i32).init(alloc);
defer list1.deinit();
try list1.append(0);
{
// Make another ArrayList with a shorter lifetime.
var list2 = std.ArrayList(i32).init(alloc);
defer list2.deinit();
// Overwrite list2 with list1. This assignment is a shallow copy.
list2 = list1;
}
// BUG: list2.deinit() has freed list1's heap storage, so reading list1 is
// now a use-after-free, and list1.deinit() will be a double free.
std.debug.print("list1 = [{}]\n", .{list1.items[0]});
} Segmentation fault at address 0x7f28a6d18000
/home/pfg/Dev/Node/temp/generated/c2eadb992fd9e98a212b0aa4ca835207/tmp/test.zig:24:52: 0x229dcd in main (test)
std.debug.print("list1 = [{}]\n", .{list1.items[0]});
^
/home/pfg/Apps/zig/zig-linux-x86_64-0.8.0-dev.2082+e9e91b4ed/lib/std/start.zig:420:37: 0x22256a in std.start.callMain (test)
const result = root.main() catch |err| {
^
/home/pfg/Apps/zig/zig-linux-x86_64-0.8.0-dev.2082+e9e91b4ed/lib/std/start.zig:362:12: 0x20653e in std.start.callMainWithArgs (test)
return @call(.{ .modifier = .always_inline }, callMain, .{});
^
/home/pfg/Apps/zig/zig-linux-x86_64-0.8.0-dev.2082+e9e91b4ed/lib/std/start.zig:316:17: 0x2057c9 in std.start.posixCallMainAndExit (test)
std.os.exit(@call(.{ .modifier = .always_inline }, callMainWithArgs, .{ argc, argv, envp }));
^
/home/pfg/Apps/zig/zig-linux-x86_64-0.8.0-dev.2082+e9e91b4ed/lib/std/start.zig:238:5: 0x2056f2 in std.start._start (test)
@call(.{ .modifier = .never_inline }, posixCallMainAndExit, .{});
^
fish: Job 1, 'zig run test.zig' terminated by signal SIGABRT (Abort) The c allocator will likely never be able to be fully safe, but there might be things that can be done. |
@pfgithub a couple questions about the GeneralPurposeAllocator if you have time: If some objects in a page are still alive, can it catch use-after-free bugs related to freed objects in the same page? Or is it only able to do that once all the objects in a page have been freed? Also, could it ever be possible to implicitly switch an application to the GeneralPurposeAllocator (or something that behaves like it) in Debug/ReleaseSafe mode, or will that sort of thing always require explicit debugging code in the application itself? Edit: Trying your examples just now, it looks like if I add an extra ArrayList that keeps an allocation of the same size alive until the end of main, it affects the debugging output. The first example doesn't catch the use-after-free, and exits without an error. The second example doesn't catch the use-after-free, but does panic on the double-free. |
The best places for questions like these is in one of the community spaces. I know that people in the discord are very responsive, and the irc is good as well especially for more technical questions about zig.
The general purpose allocator is configurable, and the default configuration Switching to a c allocator in release-fast instead of using zig's release-fast allocator does require explicit application code. |
"MemorySanitizer: fast detector of uninitialized memory use in C++" by Evgeniy Stepanov and Konstantin Serebryany https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/43308.pdf With this approach for checking UUM => 2.5x compiletime cost, 2x memory. However, this approach still includes false negatives (there can be UUM even though the check says there is none). |
Another form of UB that isn't currently safety-checked is out-of-bounds indexing into Half-baked proposal:
I like to use many-pointers instead of slices when I know I won't need to access the length at runtime. But it's sad to have to give up bounds checking in order to do that. This would be the best of both worlds. |
Quickly looking at If the field is writable from regular code, people might rely on it accidentally and the code breaks in non-debug and non-ReleaseSafe mode due to it not being available. Also nit: You forgot to mention that the Please post (from your opinion) half-baked proposals not on the issue tracker, but in a community place instead. |
To clarify, the field being secret means it's not readable or writable from user code. It's not "available" in debug builds either, so I don't see how you could accidentally rely on it. The semantics of the type wouldn't change at all, outside of the new bounds checks (and Zig doesn't have I think the idea behind a secret field has been pretty consistent across these issues, but you're right, I could have been explicit. I edited my post. Thanks for the feedback. |
if |
You both got hung up on a footnote within a footnote. I included that bit for thoroughness sake but now wish I hadn't, it's not important to the idea at all. I removed it. Anyways in my code I find many pointers/slices are dramatically more common than pointers to arrays. That's true for the compiler/stdlib too: # pointer to array
$ rg -t zig '\*(const )?\[\d+\]u8' | wc -l
92
# many pointer
rg -t zig '\[\*\](const )?u8' | wc -l
362
# slice
$ rg -t zig '\[\](const )?u8' | wc -l
2217 As you pointed out, the first, uncommon case already has a good solution. This proposal benefits the second and third, much more common cases where the length is runtime-known. |
I suppose then you're proposing that the size of slices in non-safe modes is the same size of a pointer? (this also might already be the case given |
Slices wouldn't change at all. I only mentioned them for comparison. In fast/small builds, many-pointers also wouldn't change at all. Same thing at a source code level: there are no new programmer-visible semantics (besides panics, which in Zig are not meant to be observed anyway) In debug/safe builds, the size of many-pointers would double, and a handful of builtin operations would use the secret field for bounds checking. But like Zig's other secret safety fields, the programmer can't see or access it no matter the build mode—only the compiler can during codegen. The point is to have bounds checking safety in debug builds, and space efficiency in release builds.
(Should this have been a separate issue?) |
Before we fixed this bug, every labeled piece of data was using the image of the 8 (whatever the last training item happens to be). We were referencing a stack allocated variable in the loop which just went to garbled memory after the loop ended (happened to reference the last item of the loop). It would be nice if Zig could warn/error when we tried to reference and store a pointer to some stack-allocated object to avoid this type footgun (ziglang/zig#2301).
As a special case of the |
It's always going to be possible to do unsafe things in Zig, because we have inline assembly,
@intToPtr
, and ability to call extern functions with the wrong ABI.But what if we could eliminate everything else? What if, after enumerating all kinds of undefined behavior (See #1966), we could make all of them safety-checked, with only some obvious exceptions such as inline assembly?
@intToPtr
could look at the type it was casting to and make sure it is correct, somehow?This might turn out to be impossible or impractical, but it's worth investigating.
If we could reduce the unchecked undefined behavior surface to a minimum level, there could be auditing/linting tools to point out where unsafety lies in zig software. It would be possible to say something like, "Debug and ReleaseSafe builds of Zig code are safe (in that they crash rather than have undefined behavior), except for inline assembly and extern function ABI mismatch", or some short list of exceptions.
If the cost of these protections is high, that's what we have
@setRuntimeSafety
for (see #978).It would be reasonable for these protections to depend on OS-specific behavior, and to be unavailable on some targets, such as freestanding.
The text was updated successfully, but these errors were encountered: