[v3,2/2] elf: Add a test for PT_LOAD segments with mixed p_align

Message ID 20211221031616.1534709-2-hjl.tools@gmail.com
State Superseded
Headers
Series [v3,1/2] elf: Properly align all PT_LOAD segments [BZ #28676] |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

H.J. Lu Dec. 21, 2021, 3:16 a.m. UTC
  Add tst-alignmod3-edit to edit the copy of tst-alignmod3.so to lower
p_align of the first PT_LOAD segment and verify that the shared library
is mapped with the maximum p_align of all PT_LOAD segments.
---
 elf/Makefile             |  18 ++++++
 elf/tst-alignmod3-edit.c | 122 +++++++++++++++++++++++++++++++++++++++
 elf/tst-p_align1.c       |  27 +++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 elf/tst-alignmod3-edit.c
 create mode 100644 elf/tst-p_align1.c
  

Comments

Florian Weimer Dec. 21, 2021, 8:38 a.m. UTC | #1
* H. J. Lu via Libc-alpha:

> +ifeq (yesyes,$(build-shared)$(run-built-tests))
> +tests += \
> +  tst-alignmod3-edit \
> +  tst-p_align1 \
> +
> +endif

I think you don't want to run tst-alignmod3-edit as a regular test,
although it is not do any harm, either.

> diff --git a/elf/tst-alignmod3-edit.c b/elf/tst-alignmod3-edit.c
> new file mode 100644
> index 0000000000..7b7eafbeec
> --- /dev/null
> +++ b/elf/tst-alignmod3-edit.c
> @@ -0,0 +1,122 @@

> +
> +  if (stat (file_name, &statbuf) < 0)
> +    error (1, errno, "%s: not exist \n", file_name);

Garbled message.

> +  if (statbuf.st_size < sizeof (ehdr))
> +    error (1, 0, "%s: too small\n", file_name);
> +
> +  FILE *file = fopen (file_name, "r+b");

I think you can mmap straight away, no need to use fopen/fread.

> +  /* We only support 32 bit and 64 bit ELF files.  */
> +  switch (ehdr.e_ident[EI_CLASS])
> +    {
> +    default:
> +      fclose (file);
> +      error (1, 0, "%s: unsupported ELF class: %d\n",
> +	     file_name, ehdr.e_ident[EI_CLASS]);
> +      break;
> +
> +    case ELFCLASS32:
> +    case ELFCLASS64:
> +      break;
> +    }

The code is not generic in the ELF class, it assumes the native ELF
class below (ElfW is used).  I think you should just check that the ELF
class matches the word size.

Depending on the load segments created by the linker, the test may fail
due to bug 28688 (e.g, if the first load segment already has the system
page size as p_align).

Thanks,
Florian
  
H.J. Lu Dec. 21, 2021, 3:11 p.m. UTC | #2
On Tue, Dec 21, 2021 at 12:38 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > +ifeq (yesyes,$(build-shared)$(run-built-tests))
> > +tests += \
> > +  tst-alignmod3-edit \
> > +  tst-p_align1 \
> > +
> > +endif
>
> I think you don't want to run tst-alignmod3-edit as a regular test,
> although it is not do any harm, either.

This is the easiest way to build tst-alignmod3-edit.

> > diff --git a/elf/tst-alignmod3-edit.c b/elf/tst-alignmod3-edit.c
> > new file mode 100644
> > index 0000000000..7b7eafbeec
> > --- /dev/null
> > +++ b/elf/tst-alignmod3-edit.c
> > @@ -0,0 +1,122 @@
>
> > +
> > +  if (stat (file_name, &statbuf) < 0)
> > +    error (1, errno, "%s: not exist \n", file_name);
>
> Garbled message.

Fixed.

> > +  if (statbuf.st_size < sizeof (ehdr))
> > +    error (1, 0, "%s: too small\n", file_name);
> > +
> > +  FILE *file = fopen (file_name, "r+b");
>
> I think you can mmap straight away, no need to use fopen/fread.

Fixed.

> > +  /* We only support 32 bit and 64 bit ELF files.  */
> > +  switch (ehdr.e_ident[EI_CLASS])
> > +    {
> > +    default:
> > +      fclose (file);
> > +      error (1, 0, "%s: unsupported ELF class: %d\n",
> > +          file_name, ehdr.e_ident[EI_CLASS]);
> > +      break;
> > +
> > +    case ELFCLASS32:
> > +    case ELFCLASS64:
> > +      break;
> > +    }
>
> The code is not generic in the ELF class, it assumes the native ELF
> class below (ElfW is used).  I think you should just check that the ELF
> class matches the word size.

Fixed.

> Depending on the load segments created by the linker, the test may fail
> due to bug 28688 (e.g, if the first load segment already has the system
> page size as p_align).
>

The minimum p_align of PT_LOAD segment of tst-alignmod3.so is set
by

commit 4435c29892c43ae9908a42e591747be63102689b
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Fri Dec 10 15:44:46 2021 -0800

    Support target specific ALIGN for variable alignment test [BZ #28676]

    Add <tst-file-align.h> to support target specific ALIGN for variable
    alignment test:

    1. Alpha: Use 0x10000.
    2. MicroBlaze and Nios II: Use 0x8000.
    3. All others: Use 0x200000.

I sent out the v4 patch with a check:

+ if ((phdr->p_align >> 1) < pagesize)
+   {
+     close (fd);
+     error (1, 0, "%s: p_align (0x%zx) < 2 * page size (0x%zx)",
+    file_name, phdr->p_align, 2 * pagesize);
+   }

Thanks.
  

Patch

diff --git a/elf/Makefile b/elf/Makefile
index fe42caeb0e..c3402c65b2 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -378,6 +378,13 @@  modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-auditmod18 \
 		tst-audit18mod \
 
+ifeq (yesyes,$(build-shared)$(run-built-tests))
+tests += \
+  tst-alignmod3-edit \
+  tst-p_align1 \
+
+endif
+
 # Most modules build with _ISOMAC defined, but those filtered out
 # depend on internal headers.
 modules-names-tests = $(filter-out ifuncmod% tst-tlsmod%,\
@@ -1995,3 +2002,14 @@  $(objpfx)tst-ro-dynamic-mod.so: $(objpfx)tst-ro-dynamic-mod.os \
 		$(objpfx)tst-ro-dynamic-mod.os
 
 $(objpfx)tst-rtld-run-static.out: $(objpfx)/ldconfig
+
+$(objpfx)tst-p_align1: $(objpfx)tst-p_alignmod1.so
+
+# Make a copy of tst-alignmod3.so and lower p_align of the first PT_LOAD
+# segment.
+$(objpfx)tst-p_alignmod1.so: $(objpfx)tst-alignmod3-edit \
+			     $(objpfx)tst-alignmod3.so
+	rm -f $@
+	cp $(objpfx)tst-alignmod3.so $@
+	$(test-wrapper-env) $(run-program-env) $(rtld-prefix) \
+	  $(objpfx)tst-alignmod3-edit $@
diff --git a/elf/tst-alignmod3-edit.c b/elf/tst-alignmod3-edit.c
new file mode 100644
index 0000000000..7b7eafbeec
--- /dev/null
+++ b/elf/tst-alignmod3-edit.c
@@ -0,0 +1,122 @@ 
+/* Lower p_align of the first PT_LOAD segment.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <link.h>
+#include <error.h>
+#include <errno.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+
+int
+main (int argc, char ** argv)
+{
+  if (argc != 2)
+    {
+      printf ("Usage: %s: file\n", argv[0]);
+      return 0;
+    }
+
+  const char *file_name = argv[1];
+  ElfW(Ehdr) ehdr;
+  struct stat statbuf;
+  int errno_saved;
+
+  if (stat (file_name, &statbuf) < 0)
+    error (1, errno, "%s: not exist \n", file_name);
+
+  if (statbuf.st_size < sizeof (ehdr))
+    error (1, 0, "%s: too small\n", file_name);
+
+  FILE *file = fopen (file_name, "r+b");
+  if (file == NULL)
+    error (1, 0, "%s: cann't open\n", file_name);
+
+  /* Read in the identity array.  */
+  if (fread (&ehdr, sizeof (ehdr), 1, file) != 1)
+    {
+      errno_saved = errno;
+      fclose (file);
+      error (1, errno_saved,
+	     "%s: can't read in ELF header\n", file_name);
+    }
+
+  if (ehdr.e_ident[EI_MAG0] != ELFMAG0
+      || ehdr.e_ident[EI_MAG1] != ELFMAG1
+      || ehdr.e_ident[EI_MAG2] != ELFMAG2
+      || ehdr.e_ident[EI_MAG3] != ELFMAG3)
+    {
+      fclose (file);
+      error (1, 0, "%s: bad ELF header\n", file_name);
+    }
+
+  if (ehdr.e_type != ET_DYN)
+    {
+      fclose (file);
+      error (1, 0, "%s: not shared library\n", file_name);
+    }
+
+  /* We only support 32 bit and 64 bit ELF files.  */
+  switch (ehdr.e_ident[EI_CLASS])
+    {
+    default:
+      fclose (file);
+      error (1, 0, "%s: unsupported ELF class: %d\n",
+	     file_name, ehdr.e_ident[EI_CLASS]);
+      break;
+
+    case ELFCLASS32:
+    case ELFCLASS64:
+      break;
+    }
+
+  size_t phdr_size = sizeof (ElfW(Phdr)) * ehdr.e_phentsize;
+  if (statbuf.st_size < (ehdr.e_phoff + phdr_size))
+    {
+      fclose (file);
+      error (1, 0, "%s: too small\n", file_name);
+    }
+
+  /* Map in ELF program header table.  */
+  void *base = mmap (NULL, ehdr.e_phoff + phdr_size,
+		     PROT_READ | PROT_WRITE, MAP_SHARED,
+		     fileno (file), 0);
+  if (base == MAP_FAILED)
+    {
+      errno_saved = errno;
+      fclose (file);
+      error (1, errno_saved,
+	     "%s: failed to map in program header table\n",
+	     file_name);
+    }
+
+  ElfW(Phdr) *phdr = (ElfW(Phdr) *) (base + ehdr.e_phoff);
+
+  for (int i = 0; i < ehdr.e_phnum; i++, phdr++)
+    if (phdr->p_type == PT_LOAD)
+      {
+	/* Reduce p_align of the first PT_LOAD segment by half.  */
+	phdr->p_align = phdr->p_align >> 1;
+	break;
+      }
+
+  fclose (file);
+  munmap (base, phdr_size);
+
+  return 0;
+}
diff --git a/elf/tst-p_align1.c b/elf/tst-p_align1.c
new file mode 100644
index 0000000000..cab9793220
--- /dev/null
+++ b/elf/tst-p_align1.c
@@ -0,0 +1,27 @@ 
+/* Check different alignments of PT_LOAD segments in a shared library.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+extern int do_load_test (void);
+
+static int
+do_test (void)
+{
+  return do_load_test ();
+}
+
+#include <support/test-driver.c>