Re: [PATCH] Handle DT_UNKNOWN in gconv-modules.d

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 09/06/2021 01:38, Siddhesh Poyarekar via Libc-alpha wrote:
> On filesystems that do not support dt_type, a regular file shows up as
> DT_UNKNOWN.  Fall back to using lstat64 to read file properties in
> such cases.

The patch looks ok, but two things raised checking on this code: 1.
the code is essentially the same on both places and 2. the use of 
alloca() even when it is assured that is bounded and 2. 

The former would be nice if could consolidate it (even by adding
a file where both iconvconfig and iconv could include or even
by a GLIBC_PRIVATE symbol), but it is not a deal breaker.

But I think we should move away from alloca, even when we know it
is bounded (sorry if I didn't catch on the previous patch).  For
this specific usage we can use asprintf to create the path or use
a static PATH_MAX buffer.

> ---
>  iconv/gconv_conf.c  | 9 ++++++++-
>  iconv/iconvconfig.c | 8 +++++++-
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/iconv/gconv_conf.c b/iconv/gconv_conf.c
> index c8ad8099a4..7fc3a810af 100644
> --- a/iconv/gconv_conf.c
> +++ b/iconv/gconv_conf.c
> @@ -587,7 +587,7 @@ __gconv_read_conf (void)
>  	  struct dirent *ent;
>  	  while ((ent = __readdir (confdir)) != NULL)
>  	    {
> -	      if (ent->d_type != DT_REG)
> +	      if (ent->d_type != DT_REG && ent->d_type != DT_UNKNOWN)
>  		continue;
>  
>  	      size_t len = strlen (ent->d_name);
> @@ -596,10 +596,17 @@ __gconv_read_conf (void)
>  	      if (len > strlen (suffix)
>  		  && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
>  		{
> +		  struct stat64 st;
>  		  /* 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);
> +
> +		  if (ent->d_type == DT_UNKNOWN
> +		      && (__lstat64 (conf, &st) == -1
> +			  || !S_ISREG (st.st_mode)))
> +		    continue;
> +
>  		  read_conf_file (conf, elem, elem_len, &modules, &nmodules);
>  		}
>  	    }
> diff --git a/iconv/iconvconfig.c b/iconv/iconvconfig.c
> index b2a868919c..8f10f4aba8 100644
> --- a/iconv/iconvconfig.c
> +++ b/iconv/iconvconfig.c
> @@ -747,7 +747,7 @@ handle_dir (const char *dir)
>        struct dirent *ent;
>        while ((ent = readdir (confdir)) != NULL)
>  	{
> -	  if (ent->d_type != DT_REG)
> +	  if (ent->d_type != DT_REG && ent->d_type != DT_UNKNOWN)
>  	    continue;
>  
>  	  size_t len = strlen (ent->d_name);
> @@ -756,10 +756,16 @@ handle_dir (const char *dir)
>  	  if (len > strlen (suffix)
>  	      && strcmp (ent->d_name + len - strlen (suffix), suffix) == 0)
>  	    {
> +	      struct stat64 st;
>  	      /* 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);
> +
> +	      if (ent->d_type == DT_UNKNOWN
> +		  && (lstat64 (conf, &st) == -1 || !S_ISREG (st.st_mode)))
> +		continue;
> +
>  	      found |= handle_file (dir, conf);
>  	    }
>  	}
>