Message ID | 50ea6b93-b58a-fdad-c178-a188ff6b6728@redhat.com |
---|---|
State | New |
Headers | show |
Series | Mangle NULL pointers in iconv/gconv [BZ #22025] | expand |
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
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.
*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
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.
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
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.
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 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);
Ok. Andreas.
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