diff mbox series

[1/6] iconv: Remove alloca use in gconv-modules configuration parsing

Message ID 20210610111853.2286873-2-siddhesh@sourceware.org
State Superseded
Delegated to: DJ Delorie
Headers
Series gconv configuration parsing cleanups |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Siddhesh Poyarekar June 10, 2021, 11:18 a.m. UTC 
  The alloca sizes ought to be constrained to PATH_MAX, but replace them
with dynamic allocation to be safe.  A static PATH_MAX array would
have worked too but Hurd does not have PATH_MAX and the code path is
not hot enough to micro-optimise this allocation.  Revisit if any of
those realities change.
---
 iconv/gconv_conf.c  | 17 +++++++++--------
 iconv/iconvconfig.c | 17 +++++++++++------
 2 files changed, 20 insertions(+), 14 deletions(-)

Comments

DJ Delorie June 22, 2021, 3:17 a.m. UTC | #1 
Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> The alloca sizes ought to be constrained to PATH_MAX, but replace them
> with dynamic allocation to be safe.  A static PATH_MAX array would
> have worked too but Hurd does not have PATH_MAX and the code path is
> not hot enough to micro-optimise this allocation.  Revisit if any of
> those realities change.
> ---
>  iconv/gconv_conf.c  | 17 +++++++++--------
>  iconv/iconvconfig.c | 17 +++++++++++------
>  2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
> index c8ad8099a4..3f2cef255b 100644
> --- a/iconv/gconv_conf.c
> +++ b/iconv/gconv_conf.c
> @@ -559,15 +559,15 @@ __gconv_read_conf (void)
>  
>    for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt)
>      {
> -#define BUF_LEN elem_len + sizeof (gconv_conf_dirname)
> -
>        const char *elem = __gconv_path_elem[cnt].name;
>        size_t elem_len = __gconv_path_elem[cnt].len;
> -      char *buf;
>  
>        /* No slash needs to be inserted between elem and gconv_conf_filename;
>  	 elem already ends in a slash.  */
> -      buf = alloca (BUF_LEN);
> +      char *buf = malloc (elem_len + sizeof (gconv_conf_dirname));
> +      if (buf == NULL)
> +	continue;
> +
>        char *cp = __mempcpy (__mempcpy (buf, elem, elem_len),
>  			    gconv_conf_filename, sizeof (gconv_conf_filename));
>  

replaces alloca with malloc; sizes are the same.  Ok.

> @@ -596,15 +596,16 @@ __gconv_read_conf (void)
>  	      if (len > strlen (suffix)
>  		  && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
>  		{
> -		  /* LEN <= PATH_MAX so this alloca is not unbounded.  */
> -		  char *conf = alloca (BUF_LEN + len + 1);
> -		  cp = stpcpy (conf, buf);
> -		  sprintf (cp, "/%s", ent->d_name);
> +		  char *conf;
> +		  if (__asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
> +		    continue;
>  		  read_conf_file (conf, elem, elem_len, &modules, &nmodules);
> +		  free (conf);

allocated by asprintf, free'd here.  Ok.

>  		}
>  	    }
>  	  __closedir (confdir);
>  	}
> +      free (buf);

Matched free, OK.

>      }
>  #endif
>  
> diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
> index b2a868919c..c9607fb645 100644
> --- a/iconv/iconvconfig.c
> +++ b/iconv/iconvconfig.c
> @@ -712,7 +712,6 @@ handle_file (const char *dir, const char *infile)
>  static int
>  handle_dir (const char *dir)
>  {
> -#define BUF_LEN prefix_len + dirlen + sizeof "gconv-modules.d"
>    char *cp;
>    size_t dirlen = strlen (dir);
>    bool found = false;
> @@ -726,7 +725,10 @@ handle_dir (const char *dir)
>      }
>  
>    /* First, look for a gconv-modules file.  */
> -  char buf[BUF_LEN];
> +  char *buf = malloc (prefix_len + dirlen + sizeof "gconv-modules.d");
> +  if (buf == NULL)
> +    goto out;
> +

Likewise, ok.

>    cp = buf;
>    if (dir[0] == '/')
>      cp = mempcpy (cp, prefix, prefix_len);
> @@ -756,16 +758,19 @@ handle_dir (const char *dir)
>  	  if (len > strlen (suffix)
>  	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
>  	    {
> -	      /* LEN <= PATH_MAX so this alloca is not unbounded.  */
> -	      char *conf = alloca (BUF_LEN + len + 1);
> -	      cp = stpcpy (conf, buf);
> -	      sprintf (cp, "/%s", ent->d_name);
> +	      char *conf;
> +	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
> +		continue;
>  	      found |= handle_file (dir, conf);
> +	      free (conf);

Likewise, ok.

>  	    }
>  	}
>        closedir (confdir);
>      }
>  
> +  free (buf);
> +
> +out:

Freed, ok.

>    if (!found)
>      {
>        error (0, errno, "failed to open gconv configuration files in `%s'",

LGTM.
Reviewed-by: DJ Delorie <dj@redhat.com>
diff mbox series

Patch

diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
index c8ad8099a4..3f2cef255b 100644
--- a/iconv/gconv_conf.c
+++ b/iconv/gconv_conf.c
@@ -559,15 +559,15 @@  __gconv_read_conf (void)
 
   for (cnt = 0; __gconv_path_elem[cnt].name != NULL; ++cnt)
     {
-#define BUF_LEN elem_len + sizeof (gconv_conf_dirname)
-
       const char *elem = __gconv_path_elem[cnt].name;
       size_t elem_len = __gconv_path_elem[cnt].len;
-      char *buf;
 
       /* No slash needs to be inserted between elem and gconv_conf_filename;
 	 elem already ends in a slash.  */
-      buf = alloca (BUF_LEN);
+      char *buf = malloc (elem_len + sizeof (gconv_conf_dirname));
+      if (buf == NULL)
+	continue;
+
       char *cp = __mempcpy (__mempcpy (buf, elem, elem_len),
 			    gconv_conf_filename, sizeof (gconv_conf_filename));
 
@@ -596,15 +596,16 @@  __gconv_read_conf (void)
 	      if (len > strlen (suffix)
 		  && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
 		{
-		  /* LEN <= PATH_MAX so this alloca is not unbounded.  */
-		  char *conf = alloca (BUF_LEN + len + 1);
-		  cp = stpcpy (conf, buf);
-		  sprintf (cp, "/%s", ent->d_name);
+		  char *conf;
+		  if (__asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
+		    continue;
 		  read_conf_file (conf, elem, elem_len, &modules, &nmodules);
+		  free (conf);
 		}
 	    }
 	  __closedir (confdir);
 	}
+      free (buf);
     }
 #endif
 
diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
index b2a868919c..c9607fb645 100644
--- a/iconv/iconvconfig.c
+++ b/iconv/iconvconfig.c
@@ -712,7 +712,6 @@  handle_file (const char *dir, const char *infile)
 static int
 handle_dir (const char *dir)
 {
-#define BUF_LEN prefix_len + dirlen + sizeof "gconv-modules.d"
   char *cp;
   size_t dirlen = strlen (dir);
   bool found = false;
@@ -726,7 +725,10 @@  handle_dir (const char *dir)
     }
 
   /* First, look for a gconv-modules file.  */
-  char buf[BUF_LEN];
+  char *buf = malloc (prefix_len + dirlen + sizeof "gconv-modules.d");
+  if (buf == NULL)
+    goto out;
+
   cp = buf;
   if (dir[0] == '/')
     cp = mempcpy (cp, prefix, prefix_len);
@@ -756,16 +758,19 @@  handle_dir (const char *dir)
 	  if (len > strlen (suffix)
 	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
 	    {
-	      /* LEN <= PATH_MAX so this alloca is not unbounded.  */
-	      char *conf = alloca (BUF_LEN + len + 1);
-	      cp = stpcpy (conf, buf);
-	      sprintf (cp, "/%s", ent->d_name);
+	      char *conf;
+	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
+		continue;
 	      found |= handle_file (dir, conf);
+	      free (conf);
 	    }
 	}
       closedir (confdir);
     }
 
+  free (buf);
+
+out:
   if (!found)
     {
       error (0, errno, "failed to open gconv configuration files in `%s'",