This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
[PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]
- From: Florian Weimer <fweimer at redhat dot com>
- To: GNU C Library <libc-alpha at sourceware dot org>, Patsy Franklin <pfrankli at redhat dot com>, Jeff Law <law at redhat dot com>
- Date: Tue, 29 Aug 2017 15:28:43 +0200
- Subject: [PATCH] Mangle NULL pointers in iconv/gconv [BZ #22025]
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=fweimer at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7EE328535D
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
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