Handle DT_UNKNOWN in gconv-modules.d

  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.
---
 iconv/gconv_conf.c  | 9 ++++++++-
 iconv/iconvconfig.c | 8 +++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)
  

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);
>  	    }
>  	}
>
  

On 6/9/21 10:51 PM, Adhemerval Zanella wrote:
> 
> 
> 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.

I tried to do this but the code came out clumsier because the result 
(call to handle_file vs read_conf_file) have different semantics.

> 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.

I agree, a static PATH_MAX makes sense, I'll do that.

Thanks,
Siddhesh
  

* Siddhesh Poyarekar via Libc-alpha:

>> 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.
>
> I agree, a static PATH_MAX makes sense, I'll do that.

PATH_MAX is not available on Hurd.

Thanks,
Florian
  

On 6/9/21 11:57 PM, Florian Weimer via Libc-alpha wrote:
> * Siddhesh Poyarekar via Libc-alpha:
> 
>>> 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.
>>
>> I agree, a static PATH_MAX makes sense, I'll do that.
> 
> PATH_MAX is not available on Hurd.

(grumbling redacted)