diff mbox series

libelf: Use offsetof to get field of unaligned

Message ID 20211215220544.625735-1-mark@klomp.org
State Committed
Headers show
Series libelf: Use offsetof to get field of unaligned | expand

Commit Message

Mark Wielaard Dec. 15, 2021, 10:05 p.m. UTC
gcc undefined sanitizer flags:

elf_begin.c:230:18: runtime error: member access within misaligned
address 0xf796400a for type 'struct Elf64_Shdr', which requires 4 byte
alignment struct.

This seems a wrong warning since we aren't accessing the field member
of the struct, but are taking the address of it. But we can do the
same by adding the field offsetof to the base address. Which doesn't
trigger a runtime error.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libelf/ChangeLog   |  5 +++++
 libelf/elf_begin.c | 15 +++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

Comments

Florian Weimer Dec. 15, 2021, 10:40 p.m. UTC | #1
* Mark Wielaard:

> This seems a wrong warning since we aren't accessing the field member
> of the struct, but are taking the address of it. But we can do the
> same by adding the field offsetof to the base address. Which doesn't
> trigger a runtime error.

I think the warning is correct.  I believe it is motivated by the GCC
optimizers using this to infer alignment of the original pointer.  It
won't make a difference for this expression, but it can cause crashes
elsewhere with strict-alignment targets.

Thanks,
Florian
Mark Wielaard Dec. 16, 2021, 5:12 p.m. UTC | #2
Hi Florian,

On Wed, 2021-12-15 at 23:40 +0100, Florian Weimer via Elfutils-devel
wrote:
> * Mark Wielaard:
> 
> > This seems a wrong warning since we aren't accessing the field member
> > of the struct, but are taking the address of it. But we can do the
> > same by adding the field offsetof to the base address. Which doesn't
> > trigger a runtime error.
> 
> I think the warning is correct.  I believe it is motivated by the GCC
> optimizers using this to infer alignment of the original pointer.  It
> won't make a difference for this expression, but it can cause crashes
> elsewhere with strict-alignment targets.

I think that does make sense. I tweaked to commit message a bit before
pushing the commit to say:

  We aren't actually accessing the field member of the struct, but are
  taking the address of it. Which the compiler can take as a hint that
  the address is correctly aligned. But we can do the same by adding
  the field offsetof to the base address. Which doesn't trigger a
  runtime error.

Thanks,

Mark
diff mbox series

Patch

diff --git a/libelf/ChangeLog b/libelf/ChangeLog
index 041da9b1..96059eff 100644
--- a/libelf/ChangeLog
+++ b/libelf/ChangeLog
@@ -1,3 +1,8 @@ 
+2021-12-15  Mark Wielaard  <mark@klomp.org>
+
+	* elf_begin.c (get_shnum): Use offsetof to get field of unaligned
+	struct.
+
 2021-09-06  Dmitry V. Levin  <ldv@altlinux.org>
 
 	* common.h (allocate_elf): Remove cast of calloc return value.
diff --git a/libelf/elf_begin.c b/libelf/elf_begin.c
index 93d1e12f..bd3399de 100644
--- a/libelf/elf_begin.c
+++ b/libelf/elf_begin.c
@@ -1,5 +1,6 @@ 
 /* Create descriptor for processing file.
    Copyright (C) 1998-2010, 2012, 2014, 2015, 2016 Red Hat, Inc.
+   Copyright (C) 2021 Mark J. Wielaard <mark@klomp.org>
    This file is part of elfutils.
    Written by Ulrich Drepper <drepper@redhat.com>, 1998.
 
@@ -170,9 +171,10 @@  get_shnum (void *map_address, unsigned char *e_ident, int fildes,
 	      if (likely (map_address != NULL))
 		/* gcc will optimize the memcpy to a simple memory
 		   access while taking care of alignment issues.  */
-		memcpy (&size, &((Elf32_Shdr *) ((char *) map_address
-						 + ehdr.e32->e_shoff
-						 + offset))->sh_size,
+		memcpy (&size, ((char *) map_address
+					 + ehdr.e32->e_shoff
+					 + offset
+					 + offsetof (Elf32_Shdr, sh_size)),
 			sizeof (Elf32_Word));
 	      else
 		if (unlikely ((r = pread_retry (fildes, &size,
@@ -227,9 +229,10 @@  get_shnum (void *map_address, unsigned char *e_ident, int fildes,
 	      if (likely (map_address != NULL))
 		/* gcc will optimize the memcpy to a simple memory
 		   access while taking care of alignment issues.  */
-		memcpy (&size, &((Elf64_Shdr *) ((char *) map_address
-						 + ehdr.e64->e_shoff
-						 + offset))->sh_size,
+		memcpy (&size, ((char *) map_address
+					 + ehdr.e64->e_shoff
+					 + offset
+					 + offsetof (Elf64_Shdr, sh_size)),
 			sizeof (Elf64_Xword));
 	      else
 		if (unlikely ((r = pread_retry (fildes, &size,