From fd6e2bb6b299fac363279fe57f0e2607b7c10d32 Mon Sep 17 00:00:00 2001 From: Nikita Smith Date: Wed, 9 Sep 2026 13:01:49 -0700 Subject: [PATCH] radlink: relax rules for relocations pointing into removed associative sections --- src/linker/lnk.c | 42 +++++++++++--- src/linker/lnk.h | 5 +- src/linker/lnk_config.c | 1 + src/linker/lnk_log.h | 1 + src/linker/tests/linker_tests.c | 99 +++++++++++++++++++++++++++++++++ 5 files changed, 137 insertions(+), 11 deletions(-) diff --git a/src/linker/lnk.c b/src/linker/lnk.c index b64a3219..0d908965 100644 --- a/src/linker/lnk.c +++ b/src/linker/lnk.c @@ -4392,6 +4392,11 @@ THREAD_POOL_TASK_FUNC(lnk_patch_regular_symbols_task) if (sc == task->null_sc) { section_number = lnk_obj_get_removed_section_number(obj); value = max_U32; + + COFF_ComdatSelectType selection = COFF_ComdatSelect_Null; + if (lnk_try_comdat_props_from_section_number(obj, symbol.section_number, &selection, 0, 0, 0) && selection == COFF_ComdatSelect_Associative) { + value = LNK_REMOVED_ASSOCIATIVE_SYMBOL_VALUE; + } } else { section_number = safe_cast_u32(sc->u.sect_idx + 1); value = sc->u.off + symbol.value; @@ -4429,7 +4434,7 @@ lnk_patch_obj_symtab(LNK_SymbolTable *symtab, LNK_Obj *obj, B8 *was_symbol_patch U32 value; if (was_fixup_removed || fixup_type == COFF_SymbolValueInterp_Undefined || fixup_type == COFF_SymbolValueInterp_Weak) { section_number = lnk_obj_get_removed_section_number(obj); - value = 0; + value = was_fixup_removed && fixup_src.value == LNK_REMOVED_ASSOCIATIVE_SYMBOL_VALUE ? LNK_REMOVED_ASSOCIATIVE_SYMBOL_VALUE : 0; } else { section_number = fixup_src.section_number; value = fixup_src.value; @@ -4562,16 +4567,35 @@ THREAD_POOL_TASK_FUNC(lnk_obj_reloc_patcher) COFF_SymbolValueInterpType interp = coff_interp_from_parsed_symbol(symbol); if (interp == COFF_SymbolValueInterp_Regular) { if (symbol.section_number == lnk_obj_get_removed_section_number(obj)) { - if (~section_flags & LNK_SECTION_FLAG_DEBUG) { - String8 sect_name = coff_name_from_section_header(string_table, section_header); - String8 symbol_name = lnk_symbol_name_from_coff_symbol_idx(obj, reloc->isymbol); - lnk_error_obj(LNK_Error_RelocationAgainstRemovedSection, obj, "relocating against symbol that is in a removed section (symbol: %S, reloc-section: %S 0x%llx, reloc-index: 0x%llx)", symbol_name, sect_name, it.v.section_number, reloc_idx); + + // With /OPT:REF, linkers may partially discard functions. In this case, relocations referencing those symbols must be left unchanged. + // the rule is to leave the relocations to those symbols as they are. + if (section_flags & LNK_SECTION_FLAG_DEBUG) { continue; } + + { + Temp scratch = scratch_begin(0,0); + + String8 sect_name = coff_name_from_section_header(string_table, section_header); + String8 symbol_name = lnk_symbol_name_from_coff_symbol_idx(obj, reloc->isymbol); + String8 error_message = str8f(scratch.arena, "relocating against symbol that is in a removed section (symbol: %S, reloc-section: %S 0x%llx, reloc-index: 0x%llx)", symbol_name, sect_name, it.v.section_number, reloc_idx); + + // MSVC allows relocations to reference discarded associative sections + if (symbol.value == LNK_REMOVED_ASSOCIATIVE_SYMBOL_VALUE) { + lnk_error_obj(LNK_Warning_RelocationAgainstRemovedAssociativeSection, obj, "%S", error_message); + goto next_reloc; + } + + lnk_error_obj(LNK_Error_RelocationAgainstRemovedSection, obj, "%S", error_message); + goto next_reloc; + + next_reloc:; + scratch_end(scratch); } - continue; + } else { + symbol_secnum = symbol.section_number; + symbol_secoff = symbol.value; + symbol_voff = safe_cast_u32((U64)task->image_section_table[symbol.section_number]->voff + (U64)symbol_secoff); } - symbol_secnum = symbol.section_number; - symbol_secoff = symbol.value; - symbol_voff = safe_cast_u32((U64)task->image_section_table[symbol.section_number]->voff + (U64)symbol_secoff); } else if (interp == COFF_SymbolValueInterp_Abs) { // There aren't enough bits in COFF symbol to store full image base address, // so we special case __ImageBase. A better solution would be to add diff --git a/src/linker/lnk.h b/src/linker/lnk.h index 9291e5db..a426fbac 100644 --- a/src/linker/lnk.h +++ b/src/linker/lnk.h @@ -117,8 +117,9 @@ typedef struct LNK_LinkResult // -- Image Layout ------------------------------------------------------------ -#define LNK_REMOVED_SECTION_NUMBER_32 (U32)-3 -#define LNK_REMOVED_SECTION_NUMBER_16 (U16)-3 +#define LNK_REMOVED_ASSOCIATIVE_SYMBOL_VALUE ((U32)-4) +#define LNK_REMOVED_SECTION_NUMBER_32 ((U32)-3) +#define LNK_REMOVED_SECTION_NUMBER_16 ((U16)-3) typedef struct LNK_ImageContext { diff --git a/src/linker/lnk_config.c b/src/linker/lnk_config.c index 904e5abb..1db2eba4 100644 --- a/src/linker/lnk_config.c +++ b/src/linker/lnk_config.c @@ -2845,6 +2845,7 @@ lnk_make_default_cmd_line(Arena *arena, LNK_CmdLine user_cmd_line) "/RAD_LOG:io_write", #else (char*)str8f(scratch.arena, "/RAD_IGNORE:%u", LNK_Error_InvalidTypeIndex).str, + (char*)str8f(scratch.arena, "/RAD_IGNORE:%u", LNK_Warning_RelocationAgainstRemovedAssociativeSection).str, #endif }; diff --git a/src/linker/lnk_log.h b/src/linker/lnk_log.h index b10c836b..a7fa489b 100644 --- a/src/linker/lnk_log.h +++ b/src/linker/lnk_log.h @@ -119,6 +119,7 @@ typedef enum LNK_Warning_GHash, LNK_Warning_UndefinedSectionSymbol, LNK_Warning_CyclicSymbol, + LNK_Warning_RelocationAgainstRemovedAssociativeSection, LNK_Warning_Last, LNK_Error_Count diff --git a/src/linker/tests/linker_tests.c b/src/linker/tests/linker_tests.c index 55fae91d..0772dd37 100644 --- a/src/linker/tests/linker_tests.c +++ b/src/linker/tests/linker_tests.c @@ -9841,6 +9841,105 @@ TEST(icf_cpp_multihop_functions_do_not_fold) #endif +TEST(asan_discarded_associative_metadata) +{ + // ASan can share a filename across metadata belonging to different COMDATs. + // link.exe tombstones references to a discarded associative child at RVA 0; + // it does not redirect them to the winning owner's child or retain the loser. + + U8 pointers[24] = {5}; + pointers[8] = 7; + pointers[12] = 9; + pointers[16] = 2; + pointers[20] = 11; + + for EachIndex(obj_idx, 2) { + T_Ok(t_write_def_obj(obj_idx ? "second.obj" : "first.obj", + ((T_COFF_DefObj){ + .machine = T_COFF_DefSetMachine(X64), + + .sections = (T_COFF_DefSection[]){ + {"owner", ".owner", obj_idx ? str8_lit("OWNER_B") : str8_lit("OWNER_A"), .flags = "r:data@1", .raw_flags = COFF_SectionFlag_LnkCOMDAT}, + {"meta", ".names", obj_idx ? str8_lit("SECOND.c\0") : str8_lit("FIRST.c\0"), .flags = "r:data@1", .raw_flags = COFF_SectionFlag_LnkCOMDAT}, + {"entry", ".text", str8_lit("\xC3"), .flags = "rx:code@1"}, + {obj_idx ? "pointers" : 0, ".data", str8_array_fixed(pointers), .flags = "rw:data@8", + .relocs = (T_COFF_DefReloc[]){ + T_COFF_DefReloc(X64_Addr64, 0, "filename"), + T_COFF_DefReloc(X64_Addr32Nb, 8, "filename"), + T_COFF_DefReloc(X64_SecRel, 12, "filename"), + T_COFF_DefReloc(X64_Section, 16, "filename"), + T_COFF_DefReloc(X64_Rel32, 20, "filename"), + {0} + } + }, + {0} + }, + + .symbols = (T_COFF_DefSymbol[]){ + {.type = obj_idx ? T_COFF_DefSymbol_Static : T_COFF_DefSymbol_ExternFunc, .name = "entry", .section = "entry"}, + T_COFF_DefSymbol_Secdef("owner", COFF_ComdatSelect_Any), + T_COFF_DefSymbol_Extern("shared", "owner", 0), + T_COFF_DefSymbol_Associative("meta", "owner"), + T_COFF_DefSymbol_Static("filename", "meta", 3), + {.type = obj_idx ? T_COFF_DefSymbol_Extern : 0, .name = "pointers", .section = "pointers"}, + {0} + } + + }))); + } + + for EachIndex(reverse, 2) { + for EachIndex(ref, 2) { + t_invoke_linkerf("/subsystem:console /entry:entry /nodefaultlib /fixed:no /include:shared /include:pointers /out:assoc.exe /opt:%s,noicf %s", + ref ? "ref" : "noref", + reverse ? "second.obj first.obj" : "first.obj second.obj"); + T_Ok(g_last_exit_code == 0); + + String8 image = t_read_file(arena, str8_lit("assoc.exe")); + PE_BinInfo bin = pe_bin_info_from_data(arena, image); + COFF_SectionHeader *sections = (COFF_SectionHeader *)(image.str + bin.section_table_range.min); + COFF_SectionHeader *data = 0; + COFF_SectionHeader *names = 0; + COFF_SectionHeader *owner = 0; + COFF_SectionHeader *base_relocs = 0; + U32 names_number = 0; + + for EachIndex(i, bin.section_count) { + if (MemoryMatch(sections[i].name, ".data", 6)) { data = §ions[i]; } + if (MemoryMatch(sections[i].name, ".names", 7)) { names = §ions[i]; names_number = i+1; } + if (MemoryMatch(sections[i].name, ".owner", 7)) { owner = §ions[i]; } + if (MemoryMatch(sections[i].name, ".reloc", 7)) { base_relocs = §ions[i]; } + } + + T_Ok(data && names && owner && base_relocs); + T_Ok(MemoryMatch(image.str + owner->foff, reverse ? "OWNER_B" : "OWNER_A", 7)); + T_Ok(MemoryMatch(image.str + names->foff, reverse ? "SECOND.c" : "FIRST.c", reverse ? 8 : 7)); + + U64 addr64; + U32 addr32nb, secrel, rel32; U16 section; + MemoryCopy(&addr64, image.str + data->foff, 8); + MemoryCopy(&addr32nb, image.str + data->foff + 8, 4); + MemoryCopy(&secrel, image.str + data->foff + 12, 4); + MemoryCopy(§ion, image.str + data->foff + 16, 2); + MemoryCopy(&rel32, image.str + data->foff + 20, 4); + + U32 target_voff = reverse ? names->voff + 3 : 0; + T_Ok(addr64 == bin.image_base + target_voff + 5); + T_Ok(addr32nb == target_voff + 7); + T_Ok(secrel == (reverse ? 3 : 0) + 9); + T_Ok(section == (reverse ? names_number : 0) + 2); + T_Ok(rel32 == (target_voff - data->voff - 24 + 11)); + + // Even the tombstoned ADDR64 still needs loader rebasing. + U32 page; + U16 fixup; + MemoryCopy(&page, image.str + base_relocs->foff, 4); + MemoryCopy(&fixup, image.str + base_relocs->foff + 8, 2); + T_Ok((fixup >> 12) == 10 && page + (fixup & 0xfff) == data->voff); + } + } +} + #if 0 TEST(defer_imp_link) {