[PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 454 bytes --]

We have been carrying the attached patch for a while.

There is no test because triggering this failure is very hard even on 32
bit.

Based on my review, it fixes all NULL pointer inconsistencies except the
mangling of __end_fct after a gconv_init failure.  While writing a test
for this omission I found a heap corruption, so I filed a separate bug
for these remaining issues:

  <https://sourceware.org/bugzilla/show_bug.cgi?id=22026>

Thanks,
Florian

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gconv-mangle.patch --]
[-- Type: text/x-patch; name="gconv-mangle.patch", Size: 5977 bytes --]

gconv: Consistently mangle NULL function pointers [BZ #22025]

Not mangling NULL pointers is not safe because with very low
probability, a non-NULL function pointer can turn into a NULL pointer
after mangling.

2017-08-29  Patsy Franklin  <pfrankli@redhat.com>
	    Jeff Law  <law@redhat.com>

	[BZ #22025]
	Mangle NULL pointers in iconv/gconv.
	* iconv/gconv_cache.c (find_module): Demangle init_fct before
	checking for NULL. Mangle __btowc_fct if init_fct is non-NULL.
	* iconv/gconv_db.c (free_derivation): Check that __shlib_handle
	is non-NULL before demangling the end_fct.  Check for NULL
	end_fct after demangling.
	(__gconv_release_step): Demangle the end_fct before checking
	it for NULL.   Remove assert on __shlibc_handle != NULL.
	(gen_steps): Don't check btowc_fct for NULL before mangling.
	Demangle init_fct before checking for NULL.
	(increment_counter): Likewise.
	* gconv_dl.c (__gconv_find_shlib): Don't check init_fct or
	end_fct for NULL before mangling.
	* wcsmbs/btowc.c (__btowc): Demangle btowc_fct before checking
	for NULL.

diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
index d6a47de838..7d2751a506 100644
--- a/iconv/gconv_cache.c
+++ b/iconv/gconv_cache.c
@@ -207,17 +207,16 @@ find_module (const char *directory, const char *filename,
       result->__data = NULL;
 
       /* Call the init function.  */
-      if (result->__init_fct != NULL)
-	{
-	  __gconv_init_fct init_fct = result->__init_fct;
+      __gconv_init_fct init_fct = result->__init_fct;
 #ifdef PTR_DEMANGLE
-	  PTR_DEMANGLE (init_fct);
+      PTR_DEMANGLE (init_fct);
 #endif
+      if (init_fct != NULL)
+	{
 	  status = DL_CALL_FCT (init_fct, (result));
 
 #ifdef PTR_MANGLE
-	  if (result->__btowc_fct != NULL)
-	    PTR_MANGLE (result->__btowc_fct);
+	  PTR_MANGLE (result->__btowc_fct);
 #endif
 	}
     }
diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c
index 7893fadba1..b748467de5 100644
--- a/iconv/gconv_db.c
+++ b/iconv/gconv_db.c
@@ -179,16 +179,15 @@ free_derivation (void *p)
   size_t cnt;
 
   for (cnt = 0; cnt < deriv->nsteps; ++cnt)
-    if (deriv->steps[cnt].__counter > 0
-	&& deriv->steps[cnt].__end_fct != NULL)
+    if ((deriv->steps[cnt].__counter > 0)
+	&& (deriv->steps[cnt].__shlib_handle != NULL))
       {
-	assert (deriv->steps[cnt].__shlib_handle != NULL);
-
 	__gconv_end_fct end_fct = deriv->steps[cnt].__end_fct;
 #ifdef PTR_DEMANGLE
 	PTR_DEMANGLE (end_fct);
 #endif
-	DL_CALL_FCT (end_fct, (&deriv->steps[cnt]));
+	if (end_fct != NULL)
+	  DL_CALL_FCT (end_fct, (&deriv->steps[cnt]));
       }
 
   /* Free the name strings.  */
@@ -212,16 +211,12 @@ __gconv_release_step (struct __gconv_step *step)
   if (step->__shlib_handle != NULL && --step->__counter == 0)
     {
       /* Call the destructor.  */
-      if (step->__end_fct != NULL)
-	{
-	  assert (step->__shlib_handle != NULL);
-
-	  __gconv_end_fct end_fct = step->__end_fct;
+	__gconv_end_fct end_fct = step->__end_fct;
 #ifdef PTR_DEMANGLE
-	  PTR_DEMANGLE (end_fct);
+	PTR_DEMANGLE (end_fct);
 #endif
-	  DL_CALL_FCT (end_fct, (step));
-	}
+      if (end_fct != NULL)
+	DL_CALL_FCT (end_fct, (step));
 
 #ifndef STATIC_GCONV
       /* Release the loaded module.  */
@@ -313,13 +308,11 @@ gen_steps (struct derivation_step *best, const char *toset,
 
 	      /* Call the init function.  */
 	      __gconv_init_fct init_fct = result[step_cnt].__init_fct;
-	      if (init_fct != NULL)
-		{
-		  assert (result[step_cnt].__shlib_handle != NULL);
-
 # ifdef PTR_DEMANGLE
-		  PTR_DEMANGLE (init_fct);
+	      PTR_DEMANGLE (init_fct);
 # endif
+	      if (init_fct != NULL)
+		{
 		  status = DL_CALL_FCT (init_fct, (&result[step_cnt]));
 
 		  if (__builtin_expect (status, __GCONV_OK) != __GCONV_OK)
@@ -332,8 +325,7 @@ gen_steps (struct derivation_step *best, const char *toset,
 		    }
 
 # ifdef PTR_MANGLE
-		  if (result[step_cnt].__btowc_fct != NULL)
-		    PTR_MANGLE (result[step_cnt].__btowc_fct);
+		  PTR_MANGLE (result[step_cnt].__btowc_fct);
 # endif
 		}
 	    }
@@ -415,16 +407,15 @@ increment_counter (struct __gconv_step *steps, size_t nsteps)
 
 	  /* Call the init function.  */
 	  __gconv_init_fct init_fct = step->__init_fct;
+#ifdef PTR_DEMANGLE
+	  PTR_DEMANGLE (init_fct);
+#endif
 	  if (init_fct != NULL)
 	    {
-#ifdef PTR_DEMANGLE
-	      PTR_DEMANGLE (init_fct);
-#endif
 	      DL_CALL_FCT (init_fct, (step));
 
 #ifdef PTR_MANGLE
-	      if (step->__btowc_fct != NULL)
-		PTR_MANGLE (step->__btowc_fct);
+	      PTR_MANGLE (step->__btowc_fct);
 #endif
 	    }
 	}
diff --git a/iconv/gconv_dl.c b/iconv/gconv_dl.c
index 241836204d..d7dbba90a2 100644
--- a/iconv/gconv_dl.c
+++ b/iconv/gconv_dl.c
@@ -131,10 +131,8 @@ __gconv_find_shlib (const char *name)
 
 #ifdef PTR_MANGLE
 		  PTR_MANGLE (found->fct);
-		  if (found->init_fct != NULL)
-		    PTR_MANGLE (found->init_fct);
-		  if (found->end_fct !=  NULL)
-		    PTR_MANGLE (found->end_fct);
+		  PTR_MANGLE (found->init_fct);
+		  PTR_MANGLE (found->end_fct);
 #endif
 
 		  /* We have succeeded in loading the shared object.  */
diff --git a/wcsmbs/btowc.c b/wcsmbs/btowc.c
index 22464dc5e2..97fb7170f3 100644
--- a/wcsmbs/btowc.c
+++ b/wcsmbs/btowc.c
@@ -46,15 +46,15 @@ __btowc (int c)
   /* Get the conversion functions.  */
   fcts = get_gconv_fcts (_NL_CURRENT_DATA (LC_CTYPE));
   __gconv_btowc_fct btowc_fct = fcts->towc->__btowc_fct;
+#ifdef PTR_DEMANGLE
+  if (fcts->towc->__shlib_handle != NULL)
+    PTR_DEMANGLE (btowc_fct);
+#endif
 
   if (__builtin_expect (fcts->towc_nsteps == 1, 1)
       && __builtin_expect (btowc_fct != NULL, 1))
     {
       /* Use the shortcut function.  */
-#ifdef PTR_DEMANGLE
-      if (fcts->towc->__shlib_handle != NULL)
-	PTR_DEMANGLE (btowc_fct);
-#endif
       return DL_CALL_FCT (btowc_fct, (fcts->towc, (unsigned char) c));
     }
   else

Re: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Carlos O'Donell]

On 08/29/2017 09:28 AM, Florian Weimer wrote:
> We have been carrying the attached patch for a while.
> 
> There is no test because triggering this failure is very hard even on 32
> bit.
> 
> Based on my review, it fixes all NULL pointer inconsistencies except the
> mangling of __end_fct after a gconv_init failure.  While writing a test
> for this omission I found a heap corruption, so I filed a separate bug
> for these remaining issues:
> 
>   <https://sourceware.org/bugzilla/show_bug.cgi?id=22026>

This looks good to me.

It deletes the silly NULL checking code, and makes us consistently
managle and demangle which is conceptualy easier to understand.

> gconv: Consistently mangle NULL function pointers [BZ #22025]
> 
> Not mangling NULL pointers is not safe because with very low
> probability, a non-NULL function pointer can turn into a NULL pointer
> after mangling.
> 
> 2017-08-29  Patsy Franklin  <pfrankli@redhat.com>
> 	    Jeff Law  <law@redhat.com>
> 
> 	[BZ #22025]
> 	Mangle NULL pointers in iconv/gconv.
> 	* iconv/gconv_cache.c (find_module): Demangle init_fct before
> 	checking for NULL. Mangle __btowc_fct if init_fct is non-NULL.
> 	* iconv/gconv_db.c (free_derivation): Check that __shlib_handle
> 	is non-NULL before demangling the end_fct.  Check for NULL
> 	end_fct after demangling.
> 	(__gconv_release_step): Demangle the end_fct before checking
> 	it for NULL.   Remove assert on __shlibc_handle != NULL.
> 	(gen_steps): Don't check btowc_fct for NULL before mangling.
> 	Demangle init_fct before checking for NULL.
> 	(increment_counter): Likewise.
> 	* gconv_dl.c (__gconv_find_shlib): Don't check init_fct or
> 	end_fct for NULL before mangling.
> 	* wcsmbs/btowc.c (__btowc): Demangle btowc_fct before checking
> 	for NULL.
> 
> diff --git a/iconv/gconv_cache.c b/iconv/gconv_cache.c
> index d6a47de838..7d2751a506 100644
> --- a/iconv/gconv_cache.c
> +++ b/iconv/gconv_cache.c
> @@ -207,17 +207,16 @@ find_module (const char *directory, const char *filename,
>        result->__data = NULL;
>  
>        /* Call the init function.  */
> -      if (result->__init_fct != NULL)
> -	{
> -	  __gconv_init_fct init_fct = result->__init_fct;
> +      __gconv_init_fct init_fct = result->__init_fct;
>  #ifdef PTR_DEMANGLE
> -	  PTR_DEMANGLE (init_fct);
> +      PTR_DEMANGLE (init_fct);

OK.

>  #endif
> +      if (init_fct != NULL)
> +	{
>  	  status = DL_CALL_FCT (init_fct, (result));
>  
>  #ifdef PTR_MANGLE
> -	  if (result->__btowc_fct != NULL)
> -	    PTR_MANGLE (result->__btowc_fct);
> +	  PTR_MANGLE (result->__btowc_fct);

OK.

>  #endif
>  	}
>      }
> diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c
> index 7893fadba1..b748467de5 100644
> --- a/iconv/gconv_db.c
> +++ b/iconv/gconv_db.c
> @@ -179,16 +179,15 @@ free_derivation (void *p)
>    size_t cnt;
>  
>    for (cnt = 0; cnt < deriv->nsteps; ++cnt)
> -    if (deriv->steps[cnt].__counter > 0
> -	&& deriv->steps[cnt].__end_fct != NULL)
> +    if ((deriv->steps[cnt].__counter > 0)
> +	&& (deriv->steps[cnt].__shlib_handle != NULL))

OK.

>        {
> -	assert (deriv->steps[cnt].__shlib_handle != NULL);

OK.

> -
>  	__gconv_end_fct end_fct = deriv->steps[cnt].__end_fct;
>  #ifdef PTR_DEMANGLE
>  	PTR_DEMANGLE (end_fct);
>  #endif
> -	DL_CALL_FCT (end_fct, (&deriv->steps[cnt]));
> +	if (end_fct != NULL)
> +	  DL_CALL_FCT (end_fct, (&deriv->steps[cnt]));

OK.

>        }
>  
>    /* Free the name strings.  */
> @@ -212,16 +211,12 @@ __gconv_release_step (struct __gconv_step *step)
>    if (step->__shlib_handle != NULL && --step->__counter == 0)
>      {
>        /* Call the destructor.  */
> -      if (step->__end_fct != NULL)
> -	{
> -	  assert (step->__shlib_handle != NULL);
> -
> -	  __gconv_end_fct end_fct = step->__end_fct;
> +	__gconv_end_fct end_fct = step->__end_fct;

OK.

>  #ifdef PTR_DEMANGLE
> -	  PTR_DEMANGLE (end_fct);
> +	PTR_DEMANGLE (end_fct);

OK.

>  #endif
> -	  DL_CALL_FCT (end_fct, (step));
> -	}
> +      if (end_fct != NULL)
> +	DL_CALL_FCT (end_fct, (step));

OK.

>  
>  #ifndef STATIC_GCONV
>        /* Release the loaded module.  */
> @@ -313,13 +308,11 @@ gen_steps (struct derivation_step *best, const char *toset,
>  
>  	      /* Call the init function.  */
>  	      __gconv_init_fct init_fct = result[step_cnt].__init_fct;
> -	      if (init_fct != NULL)
> -		{
> -		  assert (result[step_cnt].__shlib_handle != NULL);
> -
>  # ifdef PTR_DEMANGLE
> -		  PTR_DEMANGLE (init_fct);
> +	      PTR_DEMANGLE (init_fct);
>  # endif

OK.

> +	      if (init_fct != NULL)
> +		{
>  		  status = DL_CALL_FCT (init_fct, (&result[step_cnt]));
>  
>  		  if (__builtin_expect (status, __GCONV_OK) != __GCONV_OK)
> @@ -332,8 +325,7 @@ gen_steps (struct derivation_step *best, const char *toset,
>  		    }
>  
>  # ifdef PTR_MANGLE
> -		  if (result[step_cnt].__btowc_fct != NULL)
> -		    PTR_MANGLE (result[step_cnt].__btowc_fct);
> +		  PTR_MANGLE (result[step_cnt].__btowc_fct);

OK.

>  # endif
>  		}
>  	    }
> @@ -415,16 +407,15 @@ increment_counter (struct __gconv_step *steps, size_t nsteps)
>  
>  	  /* Call the init function.  */
>  	  __gconv_init_fct init_fct = step->__init_fct;
> +#ifdef PTR_DEMANGLE
> +	  PTR_DEMANGLE (init_fct);
> +#endif
>  	  if (init_fct != NULL)
>  	    {
> -#ifdef PTR_DEMANGLE
> -	      PTR_DEMANGLE (init_fct);
> -#endif


OK.

>  	      DL_CALL_FCT (init_fct, (step));
>  
>  #ifdef PTR_MANGLE
> -	      if (step->__btowc_fct != NULL)
> -		PTR_MANGLE (step->__btowc_fct);
> +	      PTR_MANGLE (step->__btowc_fct);

OK.

>  #endif
>  	    }
>  	}
> diff --git a/iconv/gconv_dl.c b/iconv/gconv_dl.c
> index 241836204d..d7dbba90a2 100644
> --- a/iconv/gconv_dl.c
> +++ b/iconv/gconv_dl.c
> @@ -131,10 +131,8 @@ __gconv_find_shlib (const char *name)
>  
>  #ifdef PTR_MANGLE
>  		  PTR_MANGLE (found->fct);
> -		  if (found->init_fct != NULL)
> -		    PTR_MANGLE (found->init_fct);
> -		  if (found->end_fct !=  NULL)
> -		    PTR_MANGLE (found->end_fct);
> +		  PTR_MANGLE (found->init_fct);
> +		  PTR_MANGLE (found->end_fct);

OK.

>  #endif
>  
>  		  /* We have succeeded in loading the shared object.  */
> diff --git a/wcsmbs/btowc.c b/wcsmbs/btowc.c
> index 22464dc5e2..97fb7170f3 100644
> --- a/wcsmbs/btowc.c
> +++ b/wcsmbs/btowc.c
> @@ -46,15 +46,15 @@ __btowc (int c)
>    /* Get the conversion functions.  */
>    fcts = get_gconv_fcts (_NL_CURRENT_DATA (LC_CTYPE));
>    __gconv_btowc_fct btowc_fct = fcts->towc->__btowc_fct;
> +#ifdef PTR_DEMANGLE
> +  if (fcts->towc->__shlib_handle != NULL)
> +    PTR_DEMANGLE (btowc_fct);
> +#endif

OK.

>  
>    if (__builtin_expect (fcts->towc_nsteps == 1, 1)
>        && __builtin_expect (btowc_fct != NULL, 1))
>      {
>        /* Use the shortcut function.  */
> -#ifdef PTR_DEMANGLE
> -      if (fcts->towc->__shlib_handle != NULL)
> -	PTR_DEMANGLE (btowc_fct);
> -#endif

OK.

>        return DL_CALL_FCT (btowc_fct, (fcts->towc, (unsigned char) c));
>      }
>    else

Re: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Andreas Schwab]

On Aug 29 2017, Florian Weimer <fweimer@redhat.com> wrote:

> diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c
> index 7893fadba1..b748467de5 100644
> --- a/iconv/gconv_db.c
> +++ b/iconv/gconv_db.c
> @@ -179,16 +179,15 @@ free_derivation (void *p)
>    size_t cnt;
>  
>    for (cnt = 0; cnt < deriv->nsteps; ++cnt)
> -    if (deriv->steps[cnt].__counter > 0
> -	&& deriv->steps[cnt].__end_fct != NULL)
> +    if ((deriv->steps[cnt].__counter > 0)
> +	&& (deriv->steps[cnt].__shlib_handle != NULL))

Please remove the redundant parens.

> @@ -332,8 +325,7 @@ gen_steps (struct derivation_step *best, const char *toset,
>  		    }
>  
>  # ifdef PTR_MANGLE
> -		  if (result[step_cnt].__btowc_fct != NULL)
> -		    PTR_MANGLE (result[step_cnt].__btowc_fct);
> +		  PTR_MANGLE (result[step_cnt].__btowc_fct);
>  # endif

That needs to be mangled even if there is no init_fct.

> @@ -415,16 +407,15 @@ increment_counter (struct __gconv_step *steps, size_t nsteps)
>  
>  	  /* Call the init function.  */
>  	  __gconv_init_fct init_fct = step->__init_fct;
> +#ifdef PTR_DEMANGLE
> +	  PTR_DEMANGLE (init_fct);
> +#endif
>  	  if (init_fct != NULL)
>  	    {
> -#ifdef PTR_DEMANGLE
> -	      PTR_DEMANGLE (init_fct);
> -#endif
>  	      DL_CALL_FCT (init_fct, (step));
>  
>  #ifdef PTR_MANGLE
> -	      if (step->__btowc_fct != NULL)
> -		PTR_MANGLE (step->__btowc_fct);
> +	      PTR_MANGLE (step->__btowc_fct);
>  #endif

Likewise.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Florian Weimer]

*sigh*

Right after pushing I realized that the entire premise of this patch is
bogus.

Code like this:

  /* Get the conversion functions.  */
  fcts = get_gconv_fcts (_NL_CURRENT_DATA (LC_CTYPE));
  __gconv_btowc_fct btowc_fct = fcts->towc->__btowc_fct;
#ifdef PTR_DEMANGLE
  if (fcts->towc->__shlib_handle != NULL)
    PTR_DEMANGLE (btowc_fct);
#endif

  if (__builtin_expect (fcts->towc_nsteps == 1, 1)
      && __builtin_expect (btowc_fct != NULL, 1))
    {
      /* Use the shortcut function.  */
      return DL_CALL_FCT (btowc_fct, (fcts->towc, (unsigned char) c));

provides a reasonably straightforward way for bypassing pointer
mangling, simply by setting __shlib_handle to NULL.

I'll try to come up with a different fix.

Florian

Re: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Carlos O'Donell]

On 08/29/2017 10:13 AM, Florian Weimer wrote:
> *sigh*
> 
> Right after pushing I realized that the entire premise of this patch is
> bogus.

The premise is not wrong.

The idea is to simplify the existing code to always mangle/demangle
function pointers without exception.

What you have found is a way to manipulate the mangling, which was
not considered in the original patch.
 
> Code like this:
> 
>   /* Get the conversion functions.  */
>   fcts = get_gconv_fcts (_NL_CURRENT_DATA (LC_CTYPE));
>   __gconv_btowc_fct btowc_fct = fcts->towc->__btowc_fct;
> #ifdef PTR_DEMANGLE
>   if (fcts->towc->__shlib_handle != NULL)
>     PTR_DEMANGLE (btowc_fct);
> #endif
> 
>   if (__builtin_expect (fcts->towc_nsteps == 1, 1)
>       && __builtin_expect (btowc_fct != NULL, 1))
>     {
>       /* Use the shortcut function.  */
>       return DL_CALL_FCT (btowc_fct, (fcts->towc, (unsigned char) c));
> 
> provides a reasonably straightforward way for bypassing pointer
> mangling, simply by setting __shlib_handle to NULL.

Sure, but that also has other consequences. There are several loops
which look for __shlib_handle != NULL and those loops would do nothing
if you set __shlib_handle to NULL?

> I'll try to come up with a different fix.

You do not need to come up with a different fix.

I suggest you review Andreas' comments, fixup the existing implementation,
and file a bug about the way in which the __shlib_handle might be abusable.

Don't go down the rabbit hole ;-)

Cheers,
Carlos.

Re: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Florian Weimer]

On 08/29/2017 04:18 PM, Carlos O'Donell wrote:

> I suggest you review Andreas' comments, fixup the existing implementation,
> and file a bug about the way in which the __shlib_handle might be abusable.

Right, it's a pre-existing problem.  I filed:

  https://sourceware.org/bugzilla/show_bug.cgi?id=22029

Thanks,
Florian

Re: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Carlos O'Donell]

On 08/29/2017 10:24 AM, Florian Weimer wrote:
> On 08/29/2017 04:18 PM, Carlos O'Donell wrote:
> 
>> I suggest you review Andreas' comments, fixup the existing implementation,
>> and file a bug about the way in which the __shlib_handle might be abusable.
> 
> Right, it's a pre-existing problem.  I filed:
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=22029

Exactly. Thanks for filling that bug.

c.

Re: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On 08/29/2017 03:52 PM, Andreas Schwab wrote:
> On Aug 29 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c
>> index 7893fadba1..b748467de5 100644
>> --- a/iconv/gconv_db.c
>> +++ b/iconv/gconv_db.c
>> @@ -179,16 +179,15 @@ free_derivation (void *p)
>>    size_t cnt;
>>  
>>    for (cnt = 0; cnt < deriv->nsteps; ++cnt)
>> -    if (deriv->steps[cnt].__counter > 0
>> -	&& deriv->steps[cnt].__end_fct != NULL)
>> +    if ((deriv->steps[cnt].__counter > 0)
>> +	&& (deriv->steps[cnt].__shlib_handle != NULL))
> 
> Please remove the redundant parens.
> 
>> @@ -332,8 +325,7 @@ gen_steps (struct derivation_step *best, const char *toset,
>>  		    }
>>  
>>  # ifdef PTR_MANGLE
>> -		  if (result[step_cnt].__btowc_fct != NULL)
>> -		    PTR_MANGLE (result[step_cnt].__btowc_fct);
>> +		  PTR_MANGLE (result[step_cnt].__btowc_fct);
>>  # endif
> 
> That needs to be mangled even if there is no init_fct.

Thanks.  I'm attaching a patch to fix this.  Okay?

Florian

[-- Attachment #2: gconv-mangle2.patch --]
[-- Type: text/x-patch, Size: 1837 bytes --]

iconv: Mangle __btowc_fct even without __init_fct [BZ #22025]

2017-08-29  Florian Weimer  <fweimer@redhat.com>

	[BZ #22025]
	* iconv/gconv_db.c (free_derivation): Remove redundant
	parentheses.
	(gen_steps): Unconditionally mangle __btowc_fct after
	initialization.
	(increment_counter): Likewise.  Do not call init_fct for internal
	modules.

diff --git a/iconv/gconv_db.c b/iconv/gconv_db.c
index b748467de5..7a95aeaeac 100644
--- a/iconv/gconv_db.c
+++ b/iconv/gconv_db.c
@@ -179,8 +179,8 @@ free_derivation (void *p)
   size_t cnt;
 
   for (cnt = 0; cnt < deriv->nsteps; ++cnt)
-    if ((deriv->steps[cnt].__counter > 0)
-	&& (deriv->steps[cnt].__shlib_handle != NULL))
+    if (deriv->steps[cnt].__counter > 0
+	&& deriv->steps[cnt].__shlib_handle != NULL)
       {
 	__gconv_end_fct end_fct = deriv->steps[cnt].__end_fct;
 #ifdef PTR_DEMANGLE
@@ -323,11 +323,10 @@ gen_steps (struct derivation_step *best, const char *toset,
 		      result[step_cnt].__end_fct = NULL;
 		      break;
 		    }
-
+		}
 # ifdef PTR_MANGLE
-		  PTR_MANGLE (result[step_cnt].__btowc_fct);
+	      PTR_MANGLE (result[step_cnt].__btowc_fct);
 # endif
-		}
 	    }
 	  else
 #endif
@@ -403,16 +402,14 @@ increment_counter (struct __gconv_step *steps, size_t nsteps)
 
 	      /* These settings can be overridden by the init function.  */
 	      step->__btowc_fct = NULL;
-	    }
 
-	  /* Call the init function.  */
-	  __gconv_init_fct init_fct = step->__init_fct;
+	      /* Call the init function.  */
+	      __gconv_init_fct init_fct = step->__init_fct;
 #ifdef PTR_DEMANGLE
-	  PTR_DEMANGLE (init_fct);
+	      PTR_DEMANGLE (init_fct);
 #endif
-	  if (init_fct != NULL)
-	    {
-	      DL_CALL_FCT (init_fct, (step));
+	      if (init_fct != NULL)
+		DL_CALL_FCT (init_fct, (step));
 
 #ifdef PTR_MANGLE
 	      PTR_MANGLE (step->__btowc_fct);

Re: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]

[Andreas Schwab]

Ok.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."