authorgravatar for jacobly@ziglang.orgJacob Young <jacobly@ziglang.org> 2024-02-21 16:21:14+01:00
committergravatar for andrew@ziglang.orgAndrew Kelley <andrew@ziglang.org> 2024-02-22 12:33:53-08:00
loge60d66711185fa2e3164f5af92b9786e04c4fa19
tree58a35ac4bb210ba0cfc4f781fb977d494c2c2066
parent241e100827fffde710eb0722691eeee592854744

Module: fix `@embedFile` of files containing zero bytes

If an adapted string key with embedded nulls was put in a hash map with `std.hash_map.StringIndexAdapter`, then an incorrect hash would be entered for that entry such that it is possible that when looking for the exact key that matches the prefix of the original key up to the first null would sometimes match this entry due to hash collisions and sometimes not if performed later after a grow + rehash, causing the same key to exist with two different indices breaking every string equality comparison ever, for example claiming that a container type doesn't contain a field because the field name string in the struct and the string representing the identifier to lookup might be equal strings but have different string indices. This could maybe be fixed by changing `std.hash_map.StringIndexAdapter.hash` to only hash up to the first null, therefore ensuring that the entry's hash is correct and that all future lookups will be consistent, but I don't trust anything so instead I assert that there are no embedded nulls.

6 files changed, 28 insertions(+), 18 deletions(-)

lib/std/hash_map.zig+7-10
......@@ -92,27 +92,24 @@ pub fn hashString(s: []const u8) u64 {
9292pub const StringIndexContext = struct {
9393 bytes: *const std.ArrayListUnmanaged(u8),
9494
95 pub fn eql(self: @This(), a: u32, b: u32) bool {
96 _ = self;
95 pub fn eql(_: @This(), a: u32, b: u32) bool {
9796 return a == b;
9897 }
9998
100 pub fn hash(self: @This(), x: u32) u64 {
101 const x_slice = mem.sliceTo(@as([*:0]const u8, @ptrCast(self.bytes.items.ptr)) + x, 0);
102 return hashString(x_slice);
99 pub fn hash(ctx: @This(), key: u32) u64 {
100 return hashString(mem.sliceTo(ctx.bytes.items[key..], 0));
103101 }
104102};
105103
106104pub const StringIndexAdapter = struct {
107105 bytes: *const std.ArrayListUnmanaged(u8),
108106
109 pub fn eql(self: @This(), a_slice: []const u8, b: u32) bool {
110 const b_slice = mem.sliceTo(@as([*:0]const u8, @ptrCast(self.bytes.items.ptr)) + b, 0);
111 return mem.eql(u8, a_slice, b_slice);
107 pub fn eql(ctx: @This(), a: []const u8, b: u32) bool {
108 return mem.eql(u8, a, mem.sliceTo(ctx.bytes.items[b..], 0));
112109 }
113110
114 pub fn hash(self: @This(), adapted_key: []const u8) u64 {
115 _ = self;
111 pub fn hash(_: @This(), adapted_key: []const u8) u64 {
112 assert(mem.indexOfScalar(u8, adapted_key, 0) == null);
116113 return hashString(adapted_key);
117114 }
118115};
src/AstGen.zig+6-2
......@@ -11461,6 +11461,10 @@ fn strLitAsString(astgen: *AstGen, str_lit_token: Ast.TokenIndex) !IndexSlice {
1146111461 const token_bytes = astgen.tree.tokenSlice(str_lit_token);
1146211462 try astgen.parseStrLit(str_lit_token, string_bytes, token_bytes, 0);
1146311463 const key: []const u8 = string_bytes.items[str_index..];
11464 if (std.mem.indexOfScalar(u8, key, 0)) |_| return .{
11465 .index = @enumFromInt(str_index),
11466 .len = @intCast(key.len),
11467 };
1146411468 const gop = try astgen.string_table.getOrPutContextAdapted(gpa, key, StringIndexAdapter{
1146511469 .bytes = string_bytes,
1146611470 }, StringIndexContext{
......@@ -11468,7 +11472,7 @@ fn strLitAsString(astgen: *AstGen, str_lit_token: Ast.TokenIndex) !IndexSlice {
1146811472 });
1146911473 if (gop.found_existing) {
1147011474 string_bytes.shrinkRetainingCapacity(str_index);
11471 return IndexSlice{
11475 return .{
1147211476 .index = @enumFromInt(gop.key_ptr.*),
1147311477 .len = @intCast(key.len),
1147411478 };
......@@ -11478,7 +11482,7 @@ fn strLitAsString(astgen: *AstGen, str_lit_token: Ast.TokenIndex) !IndexSlice {
1147811482 // to lookup null terminated strings, so if we get a match, it has to
1147911483 // be null terminated for that to work.
1148011484 try string_bytes.append(gpa, 0);
11481 return IndexSlice{
11485 return .{
1148211486 .index = @enumFromInt(str_index),
1148311487 .len = @intCast(key.len),
1148411488 };
src/InternPool.zig+2-1
......@@ -7985,7 +7985,8 @@ pub fn getTrailingAggregate(
79857985) Allocator.Error!Index {
79867986 try ip.items.ensureUnusedCapacity(gpa, 1);
79877987 try ip.extra.ensureUnusedCapacity(gpa, @typeInfo(Bytes).Struct.fields.len);
7988 const str: String = @enumFromInt(@intFromEnum(try getOrPutTrailingString(ip, gpa, len)));
7988
7989 const str: String = @enumFromInt(ip.string_bytes.items.len - len);
79897990 const adapter: KeyAdapter = .{ .intern_pool = ip };
79907991 const gop = try ip.map.getOrPutAdapted(gpa, Key{ .aggregate = .{
79917992 .ty = ty,
src/Module.zig+6-5
......@@ -4400,6 +4400,7 @@ fn newEmbedFile(
44004400 src_loc: SrcLoc,
44014401) !InternPool.Index {
44024402 const gpa = mod.gpa;
4403 const ip = &mod.intern_pool;
44034404
44044405 const new_file = try gpa.create(EmbedFile);
44054406 errdefer gpa.destroy(new_file);
......@@ -4414,11 +4415,11 @@ fn newEmbedFile(
44144415 .mtime = actual_stat.mtime,
44154416 };
44164417 const size = std.math.cast(usize, actual_stat.size) orelse return error.Overflow;
4417 const ip = &mod.intern_pool;
44184418
4419 const ptr = try ip.string_bytes.addManyAsSlice(gpa, size);
4420 const actual_read = try file.readAll(ptr);
4419 const bytes = try ip.string_bytes.addManyAsSlice(gpa, try std.math.add(usize, size, 1));
4420 const actual_read = try file.readAll(bytes[0..size]);
44214421 if (actual_read != size) return error.UnexpectedEndOfFile;
4422 bytes[size] = 0;
44224423
44234424 const comp = mod.comp;
44244425 switch (comp.cache_use) {
......@@ -4427,7 +4428,7 @@ fn newEmbedFile(
44274428 errdefer gpa.free(copied_resolved_path);
44284429 whole.cache_manifest_mutex.lock();
44294430 defer whole.cache_manifest_mutex.unlock();
4430 try man.addFilePostContents(copied_resolved_path, ptr, stat);
4431 try man.addFilePostContents(copied_resolved_path, bytes[0..size], stat);
44314432 },
44324433 .incremental => {},
44334434 }
......@@ -4437,7 +4438,7 @@ fn newEmbedFile(
44374438 .sentinel = .zero_u8,
44384439 .child = .u8_type,
44394440 } });
4440 const array_val = try ip.getTrailingAggregate(gpa, array_ty, size);
4441 const array_val = try ip.getTrailingAggregate(gpa, array_ty, bytes.len);
44414442
44424443 const ptr_ty = (try mod.ptrType(.{
44434444 .child = array_ty,
test/behavior.zig+7
......@@ -127,3 +127,10 @@ test {
127127 _ = @import("behavior/export_keyword.zig");
128128 }
129129}
130
131// This bug only repros in the root file
132test "deference @embedFile() of a file full of zero bytes" {
133 const contents = @embedFile("behavior/zero.bin").*;
134 try @import("std").testing.expect(contents.len == 456);
135 for (contents) |byte| try @import("std").testing.expect(byte == 0);
136}
test/behavior/zero.bin created
Binary files /dev/null and b/test/behavior/zero.bin differ