Skip to content

Commit

Permalink
i#7120: Do not add addend for rela jump slot relocation (#7121)
Browse files Browse the repository at this point in the history
Despite "rela" relocations having an explicit addend value and it being
set to non-0 in a new Debian glibc, the addend is assumed to *not* be
added to the symbol value when relocating on x86_64 and aarch64 (it does
seem to be added on RISCV).
This is not obvious and not well documented; we have to just behave like
existing loaders behave from experimentation/examination. (Yet another
reason to possibly invert the private loader and let the private copy of
ld.so do all the loading and relocating: #5437).

Tested on a machine where nearly every client test in our suite was
crashing after a glibc update: now they pass. Unfortunately it's not
simple to make automated tests for this: we don't have an existing
framework for relocation testing and it would take non-trivial effort to
construct that, beyond the scope of this fix.

Fixes #7120
  • Loading branch information
derekbruening authored Dec 11, 2024
1 parent eec4721 commit fb0542b
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 10 deletions.
12 changes: 7 additions & 5 deletions core/unix/loader.c
Original file line number Diff line number Diff line change
Expand Up @@ -787,8 +787,9 @@ privload_os_finalize(privmod_t *privmod)
GLRO_dl_tls_static_size_OFFS = last_large_load_offs;
GLRO_dl_tls_static_align_OFFS = last_large_load_offs + sizeof(void *);
LOG(GLOBAL, LOG_LOADER, 2,
"%s: for glibc 2.34+ workaround found offsets 0x%x 0x%x\n", __FUNCTION__,
GLRO_dl_tls_static_size_OFFS, GLRO_dl_tls_static_align_OFFS);
"%s: for glibc 2.34+ workaround found offsets 0x%x 0x%x for glro %p\n",
__FUNCTION__, GLRO_dl_tls_static_size_OFFS, GLRO_dl_tls_static_align_OFFS,
glro);
}
# endif
if (GLRO_dl_tls_static_size_OFFS == 0) {
Expand Down Expand Up @@ -827,7 +828,8 @@ privload_os_finalize(privmod_t *privmod)
LOG(GLOBAL, LOG_LOADER, 2, "%s: glibc 2.34+ workaround succeeded\n",
__FUNCTION__);
}
LOG(GLOBAL, LOG_LOADER, 2, "%s: calling %s\n", __FUNCTION__, LIBC_EARLY_INIT_NAME);
LOG(GLOBAL, LOG_LOADER, 2, "%s: calling %s @%p\n", __FUNCTION__, LIBC_EARLY_INIT_NAME,
libc_early_init);
(*libc_early_init)(true);
#endif /* LINUX */
}
Expand Down Expand Up @@ -1213,8 +1215,8 @@ privload_relocate_symbol(ELF_REL_TYPE *rel, os_privmod_data_t *opd, bool is_rela
uint r_type;
reg_t addend;

/* XXX: we assume ELF_REL_TYPE and ELF_RELA_TYPE only differ at the end,
* i.e. with or without r_addend.
/* ELF_REL_TYPE and ELF_RELA_TYPE differ in where the addend comes from:
* stored in the target location, or in rel->r_addend.
*/
if (is_rela)
addend = ((ELF_RELA_TYPE *)rel)->r_addend;
Expand Down
22 changes: 17 additions & 5 deletions core/unix/module_elf.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* *******************************************************************************
* Copyright (c) 2012-2022 Google, Inc. All rights reserved.
* Copyright (c) 2012-2024 Google, Inc. All rights reserved.
* Copyright (c) 2011 Massachusetts Institute of Technology All rights reserved.
* Copyright (c) 2008-2010 VMware, Inc. All rights reserved.
* *******************************************************************************/
Expand Down Expand Up @@ -1466,8 +1466,8 @@ module_relocate_symbol(ELF_REL_TYPE *rel, os_privmod_data_t *pd, bool is_rela)
const char *name;
bool resolved;

/* XXX: we assume ELF_REL_TYPE and ELF_RELA_TYPE only differ at the end,
* i.e. with or without r_addend.
/* ELF_REL_TYPE and ELF_RELA_TYPE differ in where the addend comes from:
* stored in the target location, or in rel->r_addend.
*/
if (is_rela)
addend = ((ELF_RELA_TYPE *)rel)->r_addend;
Expand All @@ -1486,7 +1486,8 @@ module_relocate_symbol(ELF_REL_TYPE *rel, os_privmod_data_t *pd, bool is_rela)
".so has relocation inside PT_DYNAMIC section");
r_type = (uint)ELF_R_TYPE(rel->r_info);

LOG(GLOBAL, LOG_LOADER, 5, "%s: reloc @ %p type=%d\n", r_addr, r_type);
LOG(GLOBAL, LOG_LOADER, 5, "%s: reloc @ %p type=%d is_rela=%d addend=0x%zx\n",
__FUNCTION__, r_addr, r_type, is_rela, addend);

/* handle the most common case, i.e. ELF_R_RELATIVE */
if (r_type == ELF_R_RELATIVE) {
Expand Down Expand Up @@ -1581,7 +1582,18 @@ module_relocate_symbol(ELF_REL_TYPE *rel, os_privmod_data_t *pd, bool is_rela)
#ifndef RISCV64 /* FIXME i#3544: Check whether ELF_R_DIRECT with !is_rela is OK */
case ELF_R_GLOB_DAT:
#endif
case ELF_R_JUMP_SLOT: *r_addr = (reg_t)res + addend; break;
case ELF_R_JUMP_SLOT:
// Neither aarch64 nor x86_64 add the addend for these types, yet riscv does.
// This is not obvious and not well documented; we have to just behave like
// existing loaders behave from experimentation/examination.
// Yet another reason to possibly invert the private loader and let
// the private copy of ld.so do all the loading and relocating: i#5437.
#if defined(AARCH64) || defined(X86)
*r_addr = (reg_t)res;
#else
*r_addr = (reg_t)res + addend;
#endif
break;
case ELF_R_DIRECT: *r_addr = (reg_t)res + (is_rela ? addend : *r_addr); break;
case ELF_R_COPY:
if (sym != NULL)
Expand Down

0 comments on commit fb0542b

Please sign in to comment.