[PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]
Carlos O'Donell
carlos@redhat.com
Tue Aug 29 13:51:00 GMT 2017
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
More information about the Libc-alpha
mailing list