Commit 56cfa8f22f69d813efedb1fa01fdcb7077ca0e5a | zig

author	Andrew Kelley <andrew@ziglang.org> 2022-08-30 17:35:25-07:00
committer	Andrew Kelley <andrew@ziglang.org> 2022-08-30 18:34:08-07:00
log	56cfa8f22f69d813efedb1fa01fdcb7077ca0e5a
tree	abefcd7eb5be87432d75e8f3abd971539a3938d3
parent	35e0ff7c364487152d786347cf70f47b2a390f12

Sema: prevent access of undefined fields

When instantiating a generic function, there is a period of time where
the function is inserted into monomorphed_funcs map, but is not yet
initialized. Despite semantic analysis being single-threaded, generic
function instantiation can happen recursively, meaning that the hash
and equality functions for monomorphed_funcs entries are potentially
invoked for an uninitialized function.

This problem was mitigated by pre-setting the hash field on the newly
allocated function, however it did not solve the problem for hash
collisions in which case the equality function would be invoked. That it
was solved for hash() but not eql() explains why the problem was
difficult to observe. I tested this patch by temporarily sabotaging the
hash and making it always return 0.

This fix is centered on adding a new field to Module.Fn which is the one
checked by eql() and is populated pre-initialization.

closes #12643

2 files changed, 26 insertions(+), 6 deletions(-)

src
- Module.zig
- Sema.zig

src/Module.zig+16

...	@@ -1496,6 +1496,22 @@ pub const Fn = struct {	...	@@ -1496,6 +1496,22 @@ pub const Fn = struct {
1496	/// active Sema context. Importantly, this value is also updated when an existing	1496	/// active Sema context. Importantly, this value is also updated when an existing
1497	/// generic function instantiation is found and called.	1497	/// generic function instantiation is found and called.
1498	branch_quota: u32,	1498	branch_quota: u32,
		1499
		1500	/// If this is not none, this function is a generic function instantiation, and
		1501	/// this is the generic function decl from which the instance was derived.
		1502	/// This information is redundant with a combination of checking if comptime_args is
		1503	/// not null and looking at the first decl dependency of owner_decl. This redundant
		1504	/// information is useful for three reasons:
		1505	/// 1. Improved perf of monomorphed_funcs when checking the eql() function because it
		1506	/// can do two fewer pointer chases by grabbing the info from this field directly
		1507	/// instead of accessing the decl and then the dependencies set.
		1508	/// 2. While a generic function instantiation is being initialized, we need hash()
		1509	/// and eql() to work before the initialization is complete. Completing the
		1510	/// insertion into the decl dependency set has more fallible operations than simply
		1511	/// setting this field.
		1512	/// 3. I forgot what the third thing was while typing up the other two.
		1513	generic_owner_decl: Decl.OptionalIndex,
		1514
1499	state: Analysis,	1515	state: Analysis,
1500	is_cold: bool = false,	1516	is_cold: bool = false,
1501	is_noinline: bool,	1517	is_noinline: bool,

src/Sema.zig+10-6

 @@ -5590,11 +5590,10 @@ const GenericCallAdapter = struct {
     pub fn eql(ctx: @This(), adapted_key: void, other_key: *Module.Fn) bool {
         _ = adapted_key;
-        // The generic function Decl is guaranteed to be the first dependency
+        // Checking for equality may happen on an item that has been inserted
-        // of each of its instantiations.
+        // into the map but is not yet fully initialized. In such case, the
-        const other_owner_decl = ctx.module.declPtr(other_key.owner_decl);
+        // two initialized fields are `hash` and `generic_owner_decl`.
-        const generic_owner_decl = other_owner_decl.dependencies.keys()[0];
+        if (ctx.generic_fn.owner_decl != other_key.generic_owner_decl.unwrap().?) return false;
-        if (ctx.generic_fn.owner_decl != generic_owner_decl) return false;
         const other_comptime_args = other_key.comptime_args.?;
         for (other_comptime_args[0..ctx.func_ty_info.param_types.len]) |other_arg, i| {
 @@ -6447,11 +6446,14 @@ fn instantiateGenericCall(
     const gop = try mod.monomorphed_funcs.getOrPutAdapted(gpa, {}, adapter);
     const callee = if (!gop.found_existing) callee: {
         const new_module_func = try gpa.create(Module.Fn);
+        errdefer gpa.destroy(new_module_func);
         // This ensures that we can operate on the hash map before the Module.Fn
         // struct is fully initialized.
         new_module_func.hash = precomputed_hash;
+        new_module_func.generic_owner_decl = module_fn.owner_decl.toOptional();
+        new_module_func.comptime_args = null;
         gop.key_ptr.* = new_module_func;
-        errdefer gpa.destroy(new_module_func);
         errdefer assert(mod.monomorphed_funcs.remove(new_module_func));
         try namespace.anon_decls.ensureUnusedCapacity(gpa, 1);
 @@ -8032,11 +8034,13 @@ fn funcCommon(
     } else null;
     const hash = new_func.hash;
+    const generic_owner_decl = if (comptime_args == null) .none else new_func.generic_owner_decl;
     const fn_payload = try sema.arena.create(Value.Payload.Function);
     new_func.* = .{
         .state = anal_state,
         .zir_body_inst = func_inst,
         .owner_decl = sema.owner_decl_index,
+        .generic_owner_decl = generic_owner_decl,
         .comptime_args = comptime_args,
         .hash = hash,
         .lbrace_line = src_locs.lbrace_line,