gconv: Replace norm_add_slashes with __gconv_norm_add_slashes

  2017-06-19  Florian Weimer  <fweimer@redhat.com>

	* iconv/Makefile (routine): Add norm_add_slashes.
	* iconv/norm_add_slashes.c: New file, extracted from
	iconv/gconv_int.h.
	* iconv/gconv_int.h (norm_add_slashes): Remove.
	(__gconv_norm_add_slashes): Declare.
	* wcsmbs/wcsmbsload.c (__wcsmbs_load_conv): Use
	__gconv_norm_add_slashes.
	* intl/dcigettext.c (_nl_find_msg): Likewise.  Simplify !_LIBC
	case.
  

On 19/06/2017 13:17, Florian Weimer wrote:
> 2017-06-19  Florian Weimer  <fweimer@redhat.com>

> diff --git a/iconv/norm_add_slashes.c b/iconv/norm_add_slashes.c
> new file mode 100644
> index 0000000..f380524
> --- /dev/null
> +++ b/iconv/norm_add_slashes.c
> @@ -0,0 +1,54 @@
> +/* Normalize the charset name and add a suffix with slashes.
> +   Copyright (C) 1997-2017 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <gconv_int.h>
> +
> +char *
> +__gconv_norm_add_slashes (const char *name, size_t name_len,
> +                          const char *suffix)
> +{
> +  const char *name_end  = name + name_len;
> +  const char *cp = name;
> +  char *result;
> +  char *tmp;
> +  size_t cnt = 0;
> +  const size_t suffix_len = strlen (suffix);
> +
> +  while (cp != name_end)
> +    if (*cp++ == '/')
> +      ++cnt;
> +
> +  tmp = result = malloc (name_len + 3 + suffix_len);
> +  if (result == NULL)
> +    return NULL;
> +  cp = name;
> +  while (cp != name_end)
> +    *tmp++ = __toupper_l (*cp++, _nl_C_locobj_ptr);
> +  if (cnt < 2)
> +    {
> +      *tmp++ = '/';
> +      if (cnt < 1)
> +        {
> +          *tmp++ = '/';
> +          if (suffix_len != 0)
> +            tmp = __mempcpy (tmp, suffix, suffix_len);
> +        }
> +    }
> +  *tmp = '\0';
> +  return result;
> +}

I think this is a good candidate for char_array usage [1]:

#define CHAR_ARRAY_INITIAL_SIZE 0
#include <malloc/char_array-skeleton.c>

char *
__gconv_norm_add_slashes (const char *name, size_t name_len,
                          const char *suffix)
{ 
  size_t cnt = 0; 
  for (size_t i = 0; i < name_len; i++)
    if (name[i] == '/')
      cnt++;
  
  struct char_array result;
  if (!char_array_init_str_size (&result, name, name_len))
    return NULL;
  
  for (size_t i = 0; i < char_array_length (&result); i++)
    *char_array_at (&result, i) = __toupper_l (name[i], _nl_C_locobj_ptr);
  
  if (cnt < 2)
    { 
      if (!char_array_append_str (&result, "/"))
        return NULL;
      if (cnt < 1)
        { 
          if (!char_array_append_str (&result, "/")
              | !char_array_append_str (&result, suffix))
            return NULL;
        }
    }
  
  return char_array_finalize (&result, NULL);
}

It has the advantages of index check of dynarray plus the malloc overflow
check.

[1] https://sourceware.org/ml/libc-alpha/2017-06/msg00325.html


> diff --git a/intl/dcigettext.c b/intl/dcigettext.c
> index d97746c..49127d0 100644
> --- a/intl/dcigettext.c
> +++ b/intl/dcigettext.c
> @@ -1123,25 +1123,24 @@ _nl_find_msg (struct loaded_l10nfile *domain_file,
>  		    {
>  		      size_t len;
>  		      char *charset;
> -		      const char *outcharset;
> +		      char *outcharset;
>  
>  		      charsetstr += strlen ("charset=");
>  		      len = strcspn (charsetstr, " \t\n");
>  
> -		      charset = (char *) alloca (len + 1);
> -# if defined _LIBC || HAVE_MEMPCPY
> -		      *((char *) mempcpy (charset, charsetstr, len)) = '\0';
> -# else
> -		      memcpy (charset, charsetstr, len);
> -		      charset[len] = '\0';
> -# endif
> -
> -		      outcharset = encoding;
> -
>  # ifdef _LIBC
>  		      /* We always want to use transliteration.  */
> -		      outcharset = norm_add_slashes (outcharset, "TRANSLIT");
> -		      charset = norm_add_slashes (charset, "");
> +		      charset = __gconv_norm_add_slashes (charsetstr, len, "");
> +		      outcharset = __gconv_norm_add_slashes
> +			(encoding, strlen (encoding),  "TRANSLIT");
> +		      if (charset == NULL || outcharset == NULL)
> +			{
> +			  free ((char *) encoding);

No need to cast on free.

> +			  free (outcharset);
> +			  free (charset);
> +			  goto unlock_fail;
> +			}
> +
>  		      int r = __gconv_open (outcharset, charset, &convd->conv,
>  					    GCONV_AVOID_NOCONV);
>  		      if (__builtin_expect (r != __GCONV_OK, 0))
> @@ -1153,6 +1152,8 @@ _nl_find_msg (struct loaded_l10nfile *domain_file,
>  			    {
>  			      gl_rwlock_unlock (domain->conversions_lock);
>  			      free ((char *) encoding);
> +			      free (outcharset);
> +			      free (charset);
>  			      return NULL;
>  			    }
>  
> @@ -1165,27 +1166,31 @@ _nl_find_msg (struct loaded_l10nfile *domain_file,
>  #   if (((__GLIBC__ == 2 && __GLIBC_MINOR__ >= 2) || __GLIBC__ > 2) \
>  	&& !defined __UCLIBC__) \
>         || _LIBICONV_VERSION >= 0x0105
> +		      charset = strndup (charsetstr, len);
>  		      if (strchr (outcharset, '/') == NULL)
>  			{
> -			  char *tmp;
> -
> -			  len = strlen (outcharset);
> -			  tmp = (char *) alloca (len + 10 + 1);
> -			  memcpy (tmp, outcharset, len);
> -			  memcpy (tmp + len, "//TRANSLIT", 10 + 1);
> -			  outcharset = tmp;
> -
> -			  convd->conv = iconv_open (outcharset, charset);
> -
> -			  freea (outcharset);
> +			  if (asprintf (&outcharset, "%s//TRANSLIT",
> +					encoding) < 0)
> +			    outcharset = NULL;
>  			}
>  		      else
> +			  outcharset = strdup (encoding);
> +		      if (charset == NULL || outcharset == NULL)
> +			{
> +			  gl_rwlock_unlock (domain->conversions_lock);
> +			  free (outcharset);
> +			  free (charset);
> +			  free ((char *) encoding);
> +			  return NULL;
> +			}
>  #   endif
> -			convd->conv = iconv_open (outcharset, charset);
> +		      convd->conv = iconv_open (outcharset, charset);
>  #  endif
>  # endif
> -
> -		      freea (charset);
> +		      free (outcharset);
> +		      free (charset);
> +		      /* Do not free encoding here because
> +                         convd->encoding takes ownership.  */
>  		    }
>  		}
>  	    }

In fact these seems another place where a char_array could find some use
to avoid all the boilerplate of managing buffers size to appendages and
buffer management (and we could get some speed up by using the stack as
well).

> diff --git a/wcsmbs/wcsmbsload.c b/wcsmbs/wcsmbsload.c
> index 656cc0a..cf7f815 100644
> --- a/wcsmbs/wcsmbsload.c
> +++ b/wcsmbs/wcsmbsload.c
> @@ -160,7 +160,6 @@ __wcsmbs_load_conv (struct __locale_data *new_category)
>      {
>        /* We must find the real functions.  */
>        const char *charset_name;
> -      const char *complete_name;
>        struct gconv_fcts *new_fcts;
>        int use_translit;
>  
> @@ -177,8 +176,11 @@ __wcsmbs_load_conv (struct __locale_data *new_category)
>  
>        /* Normalize the name and add the slashes necessary for a
>  	 complete lookup.  */
> -      complete_name = norm_add_slashes (charset_name,
> -					use_translit ? "TRANSLIT" : "");
> +      char *complete_name = __gconv_norm_add_slashes
> +	(charset_name, strlen (charset_name),
> +	 use_translit ? "TRANSLIT" : "");
> +      if (complete_name ==NULL)
> +	goto failed;
>  
>        /* It is not necessary to use transliteration in this direction
>  	 since the internal character set is supposed to be able to
> @@ -188,6 +190,7 @@ __wcsmbs_load_conv (struct __locale_data *new_category)
>        if (new_fcts->towc != NULL)
>  	new_fcts->tomb = __wcsmbs_getfct (complete_name, "INTERNAL",
>  					  &new_fcts->tomb_nsteps);
> +      free (complete_name);
>  
>        /* If any of the conversion functions is not available we don't
>  	 use any since this would mean we cannot convert back and
>
  

On 06/19/2017 11:38 PM, Adhemerval Zanella wrote:

> I think this is a good candidate for char_array usage [1]:
> 
> #define CHAR_ARRAY_INITIAL_SIZE 0
> #include <malloc/char_array-skeleton.c>
> […]

Hmm, I'll review your latest char_array patch, then.  But if we have
multiple users, we should think about out-lining more of the code.

I believe Arjun may also need a variant of this function
(norm_add_slashes) for his iconv -c fix.

>> +		      if (charset == NULL || outcharset == NULL)
>> +			{
>> +			  free ((char *) encoding);
> 
> No need to cast on free.

encoding is a const char *.

> In fact these seems another place where a char_array could find some use
> to avoid all the boilerplate of managing buffers size to appendages and
> buffer management (and we could get some speed up by using the stack as
> well).

But we can realistically make such far-reaching changes only after
eliminating the conditional compilation, and that's not something I want
to do in this patch.

Thanks,
Florian
  

On 19/06/2017 19:17, Florian Weimer wrote:
> On 06/19/2017 11:38 PM, Adhemerval Zanella wrote:
> 
>> I think this is a good candidate for char_array usage [1]:
>>
>> #define CHAR_ARRAY_INITIAL_SIZE 0
>> #include <malloc/char_array-skeleton.c>
>> […]
> 
> Hmm, I'll review your latest char_array patch, then.  But if we have
> multiple users, we should think about out-lining more of the code.

Yes, I will resend a new version with some out-lining code.

> 
> I believe Arjun may also need a variant of this function
> (norm_add_slashes) for his iconv -c fix.
> 
>>> +		      if (charset == NULL || outcharset == NULL)
>>> +			{
>>> +			  free ((char *) encoding);
>>
>> No need to cast on free.
> 
> encoding is a const char *.

Indeed... I think we can live with it for now, but this is really a bad idiom
and we should cleanup this later.

> 
>> In fact these seems another place where a char_array could find some use
>> to avoid all the boilerplate of managing buffers size to appendages and
>> buffer management (and we could get some speed up by using the stack as
>> well).
> 
> But we can realistically make such far-reaching changes only after
> eliminating the conditional compilation, and that's not something I want
> to do in this patch.

Fair enough, I also think we should focus on alloca elimination as well.