[PATCH] findtextrel: do not use unbound alloca

[PATCH] findtextrel: do not use unbound alloca

[Dmitry V. Levin]

This fixes the following compilation warning:

findtextrel.c:184:1: warning: stack usage might be unbounded [-Wstack-usage=]

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---
 src/ChangeLog     |  5 +++++
 src/Makefile.am   |  1 -
 src/findtextrel.c | 31 +++++++++----------------------
 3 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 297627df..2238916f 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,5 +1,10 @@
 2021-09-06  Dmitry V. Levin  <ldv@altlinux.org>
 
+	* findtextrel.c: Include "libeu.h".
+	(process_file): Use xasprintf instead of alloca followed by series
+	of mempcpy and stpcpy.
+	* Makefile.am (findtextrel_no_Wstack_usage): Remove.
+
 	* objdump.c (show_disasm): Replace asprintf followed by
 	error(EXIT_FAILURE) with xasprintf.
 	* readelf.c (handle_gnu_hash): Likewise.
diff --git a/src/Makefile.am b/src/Makefile.am
index 88d0ac8f..ee695d5d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -61,7 +61,6 @@ nm_no_Wstack_usage = yes
 size_no_Wstack_usage = yes
 strip_no_Wstack_usage = yes
 elflint_no_Wstack_usage = yes
-findtextrel_no_Wstack_usage = yes
 elfcmp_no_Wstack_usage = yes
 objdump_no_Wstack_usage = yes
 ranlib_no_Wstack_usage = yes
diff --git a/src/findtextrel.c b/src/findtextrel.c
index 220ee909..4bee80ae 100644
--- a/src/findtextrel.c
+++ b/src/findtextrel.c
@@ -36,6 +36,7 @@
 #include <unistd.h>
 
 #include <printversion.h>
+#include "libeu.h"
 #include "system.h"
 
 struct segments
@@ -185,25 +186,17 @@ process_file (const char *fname, bool more_than_one)
 {
   int result = 0;
   void *knownsrcs = NULL;
+  char *new_fname = NULL;
 
-  size_t fname_len = strlen (fname);
-  size_t rootdir_len = strlen (rootdir);
   const char *real_fname = fname;
   if (fname[0] == '/' && (rootdir[0] != '/' || rootdir[1] != '\0'))
-    {
-      /* Prepend the user-provided root directory.  */
-      char *new_fname = alloca (rootdir_len + fname_len + 2);
-      *((char *) mempcpy (stpcpy (mempcpy (new_fname, rootdir, rootdir_len),
-				  "/"),
-			  fname, fname_len)) = '\0';
-      real_fname = new_fname;
-    }
+    real_fname = new_fname = xasprintf ("%s/%s", rootdir, fname);
 
   int fd = open (real_fname, O_RDONLY);
   if (fd == -1)
     {
       error (0, errno, _("cannot open '%s'"), fname);
-      return 1;
+      goto err_free;
     }
 
   Elf *elf = elf_begin (fd, ELF_C_READ_MMAP, NULL);
@@ -225,6 +218,8 @@ process_file (const char *fname, bool more_than_one)
       elf_end (elf);
     err_close:
       close (fd);
+    err_free:
+      free (new_fname);
       return 1;
     }
 
@@ -362,18 +357,10 @@ cannot get program header index at offset %zd: %s"),
 	 is specified with an absolute path.  */
       if (dw == NULL && fname[0] == '/')
 	{
-	  size_t debuginfo_rootlen = strlen (debuginfo_root);
-	  char *difname = (char *) alloca (rootdir_len + debuginfo_rootlen
-					   + fname_len + 8);
-	  strcpy (mempcpy (stpcpy (mempcpy (mempcpy (difname, rootdir,
-						     rootdir_len),
-					    debuginfo_root,
-					    debuginfo_rootlen),
-				   "/"),
-			   fname, fname_len),
-		  ".debug");
-
+	  char *difname =
+	    xasprintf("%s%s/%s.debug", rootdir, debuginfo_root, fname);
 	  fd2 = open (difname, O_RDONLY);
+	  free (difname);
 	  if (fd2 != -1
 	      && (elf2 = elf_begin (fd2, ELF_C_READ_MMAP, NULL)) != NULL)
 	    dw = dwarf_begin_elf (elf2, DWARF_C_READ, NULL);
-- 
ldv

Re: [PATCH] findtextrel: do not use unbound alloca

[Mark Wielaard]

Hi Dmitry,

On Mon, Sep 06, 2021 at 06:00:00PM +0000, Dmitry V. Levin wrote:
> This fixes the following compilation warning:
> 
> findtextrel.c:184:1: warning: stack usage might be unbounded [-Wstack-usage=]

And it simplifies the code a lot IMHO.
Looks good, please apply.

Thanks,

Mark

[PATCH v2] findtextrel: do not use unbound alloca

[Dmitry V. Levin]

This fixes the following compilation warning:

findtextrel.c:184:1: warning: stack usage might be unbounded [-Wstack-usage=]

Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---

 v1 introduced a memory leak, so in v2 I rearranged the code a bit
 to make clear the new code does not introduce any memory leaks.

 src/ChangeLog     |  7 +++++++
 src/Makefile.am   |  1 -
 src/findtextrel.c | 52 +++++++++++++++++++++--------------------------
 3 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 297627df..449ca17b 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,10 @@
+2021-09-09  Dmitry V. Levin  <ldv@altlinux.org>
+
+	* findtextrel.c: Include "libeu.h".
+	(open_rootdir_file): New function.
+	(process_file): Use it to open file inside rootdir.
+	* Makefile.am (findtextrel_no_Wstack_usage): Remove.
+
 2021-09-06  Dmitry V. Levin  <ldv@altlinux.org>
 
 	* objdump.c (show_disasm): Replace asprintf followed by
diff --git a/src/Makefile.am b/src/Makefile.am
index 88d0ac8f..ee695d5d 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -61,7 +61,6 @@ nm_no_Wstack_usage = yes
 size_no_Wstack_usage = yes
 strip_no_Wstack_usage = yes
 elflint_no_Wstack_usage = yes
-findtextrel_no_Wstack_usage = yes
 elfcmp_no_Wstack_usage = yes
 objdump_no_Wstack_usage = yes
 ranlib_no_Wstack_usage = yes
diff --git a/src/findtextrel.c b/src/findtextrel.c
index 220ee909..fd7baddb 100644
--- a/src/findtextrel.c
+++ b/src/findtextrel.c
@@ -36,6 +36,7 @@
 #include <unistd.h>
 
 #include <printversion.h>
+#include "libeu.h"
 #include "system.h"
 
 struct segments
@@ -181,30 +182,31 @@ noop (void *arg __attribute__ ((unused)))
 
 
 static int
-process_file (const char *fname, bool more_than_one)
+open_rootdir_file (const char *fname)
 {
-  int result = 0;
-  void *knownsrcs = NULL;
-
-  size_t fname_len = strlen (fname);
-  size_t rootdir_len = strlen (rootdir);
+  char *new_fname = NULL;
   const char *real_fname = fname;
+
   if (fname[0] == '/' && (rootdir[0] != '/' || rootdir[1] != '\0'))
-    {
-      /* Prepend the user-provided root directory.  */
-      char *new_fname = alloca (rootdir_len + fname_len + 2);
-      *((char *) mempcpy (stpcpy (mempcpy (new_fname, rootdir, rootdir_len),
-				  "/"),
-			  fname, fname_len)) = '\0';
-      real_fname = new_fname;
-    }
+    real_fname = new_fname = xasprintf ("%s/%s", rootdir, fname);
 
   int fd = open (real_fname, O_RDONLY);
   if (fd == -1)
-    {
-      error (0, errno, _("cannot open '%s'"), fname);
-      return 1;
-    }
+    error (0, errno, _("cannot open '%s'"), fname);
+
+  free (new_fname);
+  return fd;
+}
+
+
+static int
+process_file (const char *fname, bool more_than_one)
+{
+  int result = 0;
+  void *knownsrcs = NULL;
+  int fd = open_rootdir_file (fname);
+  if (fd == -1)
+    return 1;
 
   Elf *elf = elf_begin (fd, ELF_C_READ_MMAP, NULL);
   if (elf == NULL)
@@ -362,18 +364,10 @@ cannot get program header index at offset %zd: %s"),
 	 is specified with an absolute path.  */
       if (dw == NULL && fname[0] == '/')
 	{
-	  size_t debuginfo_rootlen = strlen (debuginfo_root);
-	  char *difname = (char *) alloca (rootdir_len + debuginfo_rootlen
-					   + fname_len + 8);
-	  strcpy (mempcpy (stpcpy (mempcpy (mempcpy (difname, rootdir,
-						     rootdir_len),
-					    debuginfo_root,
-					    debuginfo_rootlen),
-				   "/"),
-			   fname, fname_len),
-		  ".debug");
-
+	  char *difname =
+	    xasprintf("%s%s/%s.debug", rootdir, debuginfo_root, fname);
 	  fd2 = open (difname, O_RDONLY);
+	  free (difname);
 	  if (fd2 != -1
 	      && (elf2 = elf_begin (fd2, ELF_C_READ_MMAP, NULL)) != NULL)
 	    dw = dwarf_begin_elf (elf2, DWARF_C_READ, NULL);
-- 
ldv

Re: [PATCH v2] findtextrel: do not use unbound alloca

[Mark Wielaard]

Hi Dmitry,

On Thu, 2021-09-09 at 14:31 +0300, Dmitry V. Levin wrote:
> This fixes the following compilation warning:
> 
> findtextrel.c:184:1: warning: stack usage might be unbounded [-
> Wstack-usage=]
> 
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
> 
>  v1 introduced a memory leak, so in v2 I rearranged the code a bit
>  to make clear the new code does not introduce any memory leaks.

Sorry I missed that in my original review. But yes, in the original you
only freed on error. Now always. Did the testsuite catch it with
configure --enable-valgrind ?

Looks good, please apply.

Thanks,

Mark
>