From ab22844176f422aeb4f523ba38f21b12d78760b3 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 20 Mar 2024 16:29:30 -0700 Subject: [PATCH 1/4] frontend: add missing bounds check for slice-by-length arrays closes #18382 --- src/Sema.zig | 16 +++++++++------ .../out of bounds array slice by length.zig | 20 +++++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 test/cases/safety/out of bounds array slice by length.zig diff --git a/src/Sema.zig b/src/Sema.zig index be3fd3e4db..6194e6c1bc 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -33284,12 +33284,16 @@ fn analyzeSlice( try sema.addSafetyCheck(block, src, is_non_null, .unwrap_null); } - if (slice_ty.isSlice(mod)) { - const slice_len_inst = try block.addTyOp(.slice_len, Type.usize, ptr_or_slice); - const actual_len = if (slice_ty.sentinel(mod) == null) - slice_len_inst - else - try sema.analyzeArithmetic(block, .add, slice_len_inst, .one, src, end_src, end_src, true); + bounds_check: { + const actual_len = if (array_ty.zigTypeTag(mod) == .Array) + try mod.intRef(Type.usize, array_ty.arrayLenIncludingSentinel(mod)) + else if (slice_ty.isSlice(mod)) l: { + const slice_len_inst = try block.addTyOp(.slice_len, Type.usize, ptr_or_slice); + break :l if (slice_ty.sentinel(mod) == null) + slice_len_inst + else + try sema.analyzeArithmetic(block, .add, slice_len_inst, .one, src, end_src, end_src, true); + } else break :bounds_check; const actual_end = if (slice_sentinel != null) try sema.analyzeArithmetic(block, .add, end, .one, src, end_src, end_src, true) diff --git a/test/cases/safety/out of bounds array slice by length.zig b/test/cases/safety/out of bounds array slice by length.zig new file mode 100644 index 0000000000..62768abebf --- /dev/null +++ b/test/cases/safety/out of bounds array slice by length.zig @@ -0,0 +1,20 @@ +const std = @import("std"); + +pub fn panic(message: []const u8, stack_trace: ?*std.builtin.StackTrace, _: ?usize) noreturn { + _ = stack_trace; + if (std.mem.eql(u8, message, "index out of bounds: index 16, len 5")) { + std.process.exit(0); + } + std.process.exit(1); +} +pub fn main() !void { + var buf: [5]u8 = undefined; + _ = buf[foo(6)..][0..10]; + return error.TestFailed; +} +fn foo(a: u32) u32 { + return a; +} +// run +// backend=llvm +// target=native From 2583b389eaf5f7aaa0eb79b51126506c1e172d15 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 20 Mar 2024 17:02:35 -0700 Subject: [PATCH 2/4] frontend: comptime array slice-by-length OOB detection --- src/Sema.zig | 37 ++++++++++++++----- .../out of bounds array slice by length.zig | 15 ++++++++ .../out of bounds array slice by length.zig | 4 +- 3 files changed, 45 insertions(+), 11 deletions(-) create mode 100644 test/cases/compile_errors/out of bounds array slice by length.zig diff --git a/src/Sema.zig b/src/Sema.zig index 6194e6c1bc..764aa95ebe 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -33285,15 +33285,34 @@ fn analyzeSlice( } bounds_check: { - const actual_len = if (array_ty.zigTypeTag(mod) == .Array) - try mod.intRef(Type.usize, array_ty.arrayLenIncludingSentinel(mod)) - else if (slice_ty.isSlice(mod)) l: { - const slice_len_inst = try block.addTyOp(.slice_len, Type.usize, ptr_or_slice); - break :l if (slice_ty.sentinel(mod) == null) - slice_len_inst - else - try sema.analyzeArithmetic(block, .add, slice_len_inst, .one, src, end_src, end_src, true); - } else break :bounds_check; + const actual_len = l: { + if (array_ty.zigTypeTag(mod) == .Array) { + const len = array_ty.arrayLenIncludingSentinel(mod); + // If the end is comptime-known, we can emit a + // compile error if it would be out-of-bounds even + // with a start value of 0. + if (uncasted_end_opt != .none) { + if (try sema.resolveDefinedValue(block, end_src, uncasted_end_opt)) |end_val| { + const end_int = end_val.getUnsignedInt(mod).?; + if (end_int > len) return sema.fail( + block, + end_src, + "slice end index {d} exceeds array length of type '{}'", + .{ end_int, array_ty.fmt(mod) }, + ); + } + } + break :l try mod.intRef(Type.usize, len); + } + if (slice_ty.isSlice(mod)) { + const slice_len_inst = try block.addTyOp(.slice_len, Type.usize, ptr_or_slice); + break :l if (slice_ty.sentinel(mod) == null) + slice_len_inst + else + try sema.analyzeArithmetic(block, .add, slice_len_inst, .one, src, end_src, end_src, true); + } + break :bounds_check; + }; const actual_end = if (slice_sentinel != null) try sema.analyzeArithmetic(block, .add, end, .one, src, end_src, end_src, true) diff --git a/test/cases/compile_errors/out of bounds array slice by length.zig b/test/cases/compile_errors/out of bounds array slice by length.zig new file mode 100644 index 0000000000..75ed52eb7a --- /dev/null +++ b/test/cases/compile_errors/out of bounds array slice by length.zig @@ -0,0 +1,15 @@ +export fn b() void { + var buf: [5]u8 = undefined; + _ = buf[foo(6)..][0..10]; + return error.TestFailed; +} + +fn foo(a: u32) u32 { + return a; +} + +// error +// backend=stage2 +// target=native +// +// :3:26: error: slice end index 10 exceeds array length of type '[5]u8' diff --git a/test/cases/safety/out of bounds array slice by length.zig b/test/cases/safety/out of bounds array slice by length.zig index 62768abebf..0c1de92fff 100644 --- a/test/cases/safety/out of bounds array slice by length.zig +++ b/test/cases/safety/out of bounds array slice by length.zig @@ -2,14 +2,14 @@ const std = @import("std"); pub fn panic(message: []const u8, stack_trace: ?*std.builtin.StackTrace, _: ?usize) noreturn { _ = stack_trace; - if (std.mem.eql(u8, message, "index out of bounds: index 16, len 5")) { + if (std.mem.eql(u8, message, "index out of bounds: index 9, len 5")) { std.process.exit(0); } std.process.exit(1); } pub fn main() !void { var buf: [5]u8 = undefined; - _ = buf[foo(6)..][0..10]; + _ = buf[foo(6)..][0..3]; return error.TestFailed; } fn foo(a: u32) u32 { From 7741aca96c8cc6df7e8c4bd10ada741d6a3ffb9d Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 20 Mar 2024 17:26:31 -0700 Subject: [PATCH 3/4] std: work around compiler unable to evaluate condition at compile time --- lib/std/json/static.zig | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/lib/std/json/static.zig b/lib/std/json/static.zig index ea0bb6c0f2..312fa1a119 100644 --- a/lib/std/json/static.zig +++ b/lib/std/json/static.zig @@ -400,24 +400,20 @@ pub fn innerParse( @memcpy(r[i..][0..slice.len], slice); i += slice.len; }, - .partial_string_escaped_1 => |arr| { + inline .partial_string_escaped_1, + .partial_string_escaped_2, + .partial_string_escaped_3, + .partial_string_escaped_4, + => |arr| { if (i + arr.len > r.len) return error.LengthMismatch; - @memcpy(r[i..][0..arr.len], arr[0..]); - i += arr.len; - }, - .partial_string_escaped_2 => |arr| { - if (i + arr.len > r.len) return error.LengthMismatch; - @memcpy(r[i..][0..arr.len], arr[0..]); - i += arr.len; - }, - .partial_string_escaped_3 => |arr| { - if (i + arr.len > r.len) return error.LengthMismatch; - @memcpy(r[i..][0..arr.len], arr[0..]); - i += arr.len; - }, - .partial_string_escaped_4 => |arr| { - if (i + arr.len > r.len) return error.LengthMismatch; - @memcpy(r[i..][0..arr.len], arr[0..]); + + // Implementing https://github.com/ziglang/zig/issues/3806 + // would make this no longer needed because the + // above condition would become compile-time + // known. + if (arr.len > r.len) unreachable; + + @memcpy(r[i..][0..arr.len], &arr); i += arr.len; }, else => unreachable, From 4d5e0a0434a69059f623e5c51862fb1a2c553abc Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Wed, 20 Mar 2024 17:28:51 -0700 Subject: [PATCH 4/4] Revert the last two commits in this branch When the slice-by-length start position is runtime-known, it is likely protected by a runtime-known condition and therefore a compile error is less appropriate than a runtime panic check. This is demonstrated in the json code that was updated and then reverted in this commit. When #3806 is implemented, this decision can be reassessed. Revert "std: work around compiler unable to evaluate condition at compile time" Revert "frontend: comptime array slice-by-length OOB detection" This reverts commit 7741aca96c8cc6df7e8c4bd10ada741d6a3ffb9d. This reverts commit 2583b389eaf5f7aaa0eb79b51126506c1e172d15. --- lib/std/json/static.zig | 30 ++++++++------- src/Sema.zig | 37 +++++-------------- .../out of bounds array slice by length.zig | 15 -------- .../out of bounds array slice by length.zig | 4 +- 4 files changed, 28 insertions(+), 58 deletions(-) delete mode 100644 test/cases/compile_errors/out of bounds array slice by length.zig diff --git a/lib/std/json/static.zig b/lib/std/json/static.zig index 312fa1a119..ea0bb6c0f2 100644 --- a/lib/std/json/static.zig +++ b/lib/std/json/static.zig @@ -400,20 +400,24 @@ pub fn innerParse( @memcpy(r[i..][0..slice.len], slice); i += slice.len; }, - inline .partial_string_escaped_1, - .partial_string_escaped_2, - .partial_string_escaped_3, - .partial_string_escaped_4, - => |arr| { + .partial_string_escaped_1 => |arr| { if (i + arr.len > r.len) return error.LengthMismatch; - - // Implementing https://github.com/ziglang/zig/issues/3806 - // would make this no longer needed because the - // above condition would become compile-time - // known. - if (arr.len > r.len) unreachable; - - @memcpy(r[i..][0..arr.len], &arr); + @memcpy(r[i..][0..arr.len], arr[0..]); + i += arr.len; + }, + .partial_string_escaped_2 => |arr| { + if (i + arr.len > r.len) return error.LengthMismatch; + @memcpy(r[i..][0..arr.len], arr[0..]); + i += arr.len; + }, + .partial_string_escaped_3 => |arr| { + if (i + arr.len > r.len) return error.LengthMismatch; + @memcpy(r[i..][0..arr.len], arr[0..]); + i += arr.len; + }, + .partial_string_escaped_4 => |arr| { + if (i + arr.len > r.len) return error.LengthMismatch; + @memcpy(r[i..][0..arr.len], arr[0..]); i += arr.len; }, else => unreachable, diff --git a/src/Sema.zig b/src/Sema.zig index 764aa95ebe..6194e6c1bc 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -33285,34 +33285,15 @@ fn analyzeSlice( } bounds_check: { - const actual_len = l: { - if (array_ty.zigTypeTag(mod) == .Array) { - const len = array_ty.arrayLenIncludingSentinel(mod); - // If the end is comptime-known, we can emit a - // compile error if it would be out-of-bounds even - // with a start value of 0. - if (uncasted_end_opt != .none) { - if (try sema.resolveDefinedValue(block, end_src, uncasted_end_opt)) |end_val| { - const end_int = end_val.getUnsignedInt(mod).?; - if (end_int > len) return sema.fail( - block, - end_src, - "slice end index {d} exceeds array length of type '{}'", - .{ end_int, array_ty.fmt(mod) }, - ); - } - } - break :l try mod.intRef(Type.usize, len); - } - if (slice_ty.isSlice(mod)) { - const slice_len_inst = try block.addTyOp(.slice_len, Type.usize, ptr_or_slice); - break :l if (slice_ty.sentinel(mod) == null) - slice_len_inst - else - try sema.analyzeArithmetic(block, .add, slice_len_inst, .one, src, end_src, end_src, true); - } - break :bounds_check; - }; + const actual_len = if (array_ty.zigTypeTag(mod) == .Array) + try mod.intRef(Type.usize, array_ty.arrayLenIncludingSentinel(mod)) + else if (slice_ty.isSlice(mod)) l: { + const slice_len_inst = try block.addTyOp(.slice_len, Type.usize, ptr_or_slice); + break :l if (slice_ty.sentinel(mod) == null) + slice_len_inst + else + try sema.analyzeArithmetic(block, .add, slice_len_inst, .one, src, end_src, end_src, true); + } else break :bounds_check; const actual_end = if (slice_sentinel != null) try sema.analyzeArithmetic(block, .add, end, .one, src, end_src, end_src, true) diff --git a/test/cases/compile_errors/out of bounds array slice by length.zig b/test/cases/compile_errors/out of bounds array slice by length.zig deleted file mode 100644 index 75ed52eb7a..0000000000 --- a/test/cases/compile_errors/out of bounds array slice by length.zig +++ /dev/null @@ -1,15 +0,0 @@ -export fn b() void { - var buf: [5]u8 = undefined; - _ = buf[foo(6)..][0..10]; - return error.TestFailed; -} - -fn foo(a: u32) u32 { - return a; -} - -// error -// backend=stage2 -// target=native -// -// :3:26: error: slice end index 10 exceeds array length of type '[5]u8' diff --git a/test/cases/safety/out of bounds array slice by length.zig b/test/cases/safety/out of bounds array slice by length.zig index 0c1de92fff..62768abebf 100644 --- a/test/cases/safety/out of bounds array slice by length.zig +++ b/test/cases/safety/out of bounds array slice by length.zig @@ -2,14 +2,14 @@ const std = @import("std"); pub fn panic(message: []const u8, stack_trace: ?*std.builtin.StackTrace, _: ?usize) noreturn { _ = stack_trace; - if (std.mem.eql(u8, message, "index out of bounds: index 9, len 5")) { + if (std.mem.eql(u8, message, "index out of bounds: index 16, len 5")) { std.process.exit(0); } std.process.exit(1); } pub fn main() !void { var buf: [5]u8 = undefined; - _ = buf[foo(6)..][0..3]; + _ = buf[foo(6)..][0..10]; return error.TestFailed; } fn foo(a: u32) u32 {