Skip to content

radlink: match MSVC for discarded associative COMDAT relocations #940

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.

Sign up for GitHub

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

Closed
honkstar1 wants to merge 1 commit into from
+100 −11
Closed

radlink: match MSVC for discarded associative COMDAT relocations #940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions src/linker/lnk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4389,9 +4389,14 @@ THREAD_POOL_TASK_FUNC(lnk_patch_regular_symbols_task)
LNK_SectionContrib *sc = task->sect_map[obj_idx][symbol.section_number];
U32 section_number;
U32 value;
if (sc == task->null_sc) {
section_number = lnk_obj_get_removed_section_number(obj);
value = max_U32;
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;
Expand Down Expand Up @@ -4427,9 +4432,10 @@ lnk_patch_obj_symtab(LNK_SymbolTable *symtab, LNK_Obj *obj, B8 *was_symbol_patch

U32 section_number;
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;
if (was_fixup_removed || fixup_type == COFF_SymbolValueInterp_Undefined || fixup_type == COFF_SymbolValueInterp_Weak) {
section_number = lnk_obj_get_removed_section_number(obj);
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;
Expand Down Expand Up @@ -4562,16 +4568,22 @@ 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) {
if (section_flags & LNK_SECTION_FLAG_DEBUG) { continue; }
if (symbol.value != LNK_REMOVED_ASSOCIATIVE_SYMBOL_VALUE) {
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);
continue;
}
continue;
// MSVC permits references to discarded associative metadata (ASan
// filename strings can be shared across different COMDAT owners).
// Use a zero target RVA/section/offset, retaining the relocation's
// normal addend and image-base adjustment. Do not revive the section.
} 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
Expand Down
2 changes: 2 additions & 0 deletions src/linker/lnk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,8 @@ typedef struct LNK_LinkResult

#define LNK_REMOVED_SECTION_NUMBER_32 (U32)-3
#define LNK_REMOVED_SECTION_NUMBER_16 (U16)-3
// Internal tag carried through symbol fixups, not an output address.
#define LNK_REMOVED_ASSOCIATIVE_SYMBOL_VALUE (max_U32 - 1)

typedef struct LNK_ImageContext
{
Expand Down
75 changes: 75 additions & 0 deletions src/linker/tests/linker_tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4106,6 +4106,81 @@ TEST(comdat_largest)
}
}

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 (U32 obj_idx = 0; obj_idx < 2; obj_idx += 1) {
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},
{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}}},
{"entry", ".text", str8_lit("\xC3"), .flags = "rx:code@1"}, {0}},
.symbols = (T_COFF_DefSymbol[]){
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"},
T_COFF_DefSymbol_ExternFunc("entry", "entry", 0), {0}},
}));
}
for (U32 reverse = 0; reverse < 2; reverse += 1) {
for (U32 ref = 0; ref < 2; ref += 1) {
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, *names = 0, *owner = 0, *base_relocs = 0;
U32 names_number = 0;
for EachIndex(i, bin.section_count) {
if (MemoryMatch(sections[i].name, ".data", 6)) { data = &sections[i]; }
if (MemoryMatch(sections[i].name, ".names", 7)) { names = &sections[i]; names_number = i+1; }
if (MemoryMatch(sections[i].name, ".owner", 7)) { owner = &sections[i]; }
if (MemoryMatch(sections[i].name, ".reloc", 7)) { base_relocs = &sections[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(&section, 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 == (U32)(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);
}
}
}

TEST(comdat_associative)
{
T_Ok(t_write_def_obj("a.obj", (T_COFF_DefObj){
Expand Down