| Submitter | Eric Blake |
|---|---|
| Date | Dec. 6, 2018, 3:18 p.m. |
| Message ID | <20181206151856.77503-1-eblake@redhat.com> |
| Download | mbox | patch |
| Permalink | /patch/674223/ |
| State | New |
| Headers | show |
Comments
Le 06/12/2018 à 16:18, Eric Blake a écrit : > POSIX states that the value of endptr is unspecified if strtol() > fails with EINVAL due to an invalid base argument. Since none of > the callers to check_strtox_error() initialized endptr, we could > end up propagating uninitialized data back to a caller on error. > However, passing an out-of-range base is already a sign of poor > programming, so let's just assert that base is in range, at which > point check_strtox_error() can be tightened to assert that it is > receiving an initialized ep that points somewhere within the > caller's original string, regardless of whether strto*() succeeded > or failed with ERANGE. > > Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > Also tested that this does not negatively impact David's pending > additions of qemu_strtod{,_finite}(). Thus: > Based-on: <20181121164421.20780-1-david@redhat.com> > > util/cutils.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/util/cutils.c b/util/cutils.c > index 91930d1bbeb..e098debdc0c 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -278,6 +278,7 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result) > static int check_strtox_error(const char *nptr, char *ep, > const char **endptr, int libc_errno) > { > + assert(ep >= nptr); > if (endptr) { > *endptr = ep; > } > @@ -325,6 +326,7 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base, > char *ep; > long long lresult; > > + assert((unsigned) base <= 36 && base != 1); If you want to play with type, I think you can do: assert((unsigned)(base - 2) <= 34) Thanks, Laurent
Le 06/12/2018 à 17:40, Laurent Vivier a écrit : > Le 06/12/2018 à 16:18, Eric Blake a écrit : >> POSIX states that the value of endptr is unspecified if strtol() >> fails with EINVAL due to an invalid base argument. Since none of >> the callers to check_strtox_error() initialized endptr, we could >> end up propagating uninitialized data back to a caller on error. >> However, passing an out-of-range base is already a sign of poor >> programming, so let's just assert that base is in range, at which >> point check_strtox_error() can be tightened to assert that it is >> receiving an initialized ep that points somewhere within the >> caller's original string, regardless of whether strto*() succeeded >> or failed with ERANGE. >> >> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> >> Also tested that this does not negatively impact David's pending >> additions of qemu_strtod{,_finite}(). Thus: >> Based-on: <20181121164421.20780-1-david@redhat.com> >> >> util/cutils.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/util/cutils.c b/util/cutils.c >> index 91930d1bbeb..e098debdc0c 100644 >> --- a/util/cutils.c >> +++ b/util/cutils.c >> @@ -278,6 +278,7 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result) >> static int check_strtox_error(const char *nptr, char *ep, >> const char **endptr, int libc_errno) >> { >> + assert(ep >= nptr); >> if (endptr) { >> *endptr = ep; >> } >> @@ -325,6 +326,7 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base, >> char *ep; >> long long lresult; >> >> + assert((unsigned) base <= 36 && base != 1); > > If you want to play with type, I think you can do: > > assert((unsigned)(base - 2) <= 34) oops, no, '0' is a valid case. Forgive this... Laurent
Eric Blake <eblake@redhat.com> writes: > POSIX states that the value of endptr is unspecified if strtol() > fails with EINVAL due to an invalid base argument. Since none of > the callers to check_strtox_error() initialized endptr, we could > end up propagating uninitialized data back to a caller on error. > However, passing an out-of-range base is already a sign of poor > programming, so let's just assert that base is in range, at which > point check_strtox_error() can be tightened to assert that it is > receiving an initialized ep that points somewhere within the > caller's original string, regardless of whether strto*() succeeded > or failed with ERANGE. > > Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 12/6/18 4:18 PM, Eric Blake wrote: > POSIX states that the value of endptr is unspecified if strtol() > fails with EINVAL due to an invalid base argument. Since none of > the callers to check_strtox_error() initialized endptr, we could > end up propagating uninitialized data back to a caller on error. > However, passing an out-of-range base is already a sign of poor > programming, so let's just assert that base is in range, at which > point check_strtox_error() can be tightened to assert that it is > receiving an initialized ep that points somewhere within the > caller's original string, regardless of whether strto*() succeeded > or failed with ERANGE. > > Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > Also tested that this does not negatively impact David's pending > additions of qemu_strtod{,_finite}(). Thus: > Based-on: <20181121164421.20780-1-david@redhat.com> > > util/cutils.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/util/cutils.c b/util/cutils.c > index 91930d1bbeb..e098debdc0c 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -278,6 +278,7 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result) > static int check_strtox_error(const char *nptr, char *ep, > const char **endptr, int libc_errno) > { > + assert(ep >= nptr); > if (endptr) { > *endptr = ep; > } > @@ -325,6 +326,7 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base, > char *ep; > long long lresult; > > + assert((unsigned) base <= 36 && base != 1); > if (!nptr) { > if (endptr) { > *endptr = nptr; > @@ -377,6 +379,7 @@ int qemu_strtoui(const char *nptr, const char **endptr, int base, > char *ep; > long long lresult; > > + assert((unsigned) base <= 36 && base != 1); > if (!nptr) { > if (endptr) { > *endptr = nptr; > @@ -433,6 +436,7 @@ int qemu_strtol(const char *nptr, const char **endptr, int base, > { > char *ep; > > + assert((unsigned) base <= 36 && base != 1); > if (!nptr) { > if (endptr) { > *endptr = nptr; > @@ -475,6 +479,7 @@ int qemu_strtoul(const char *nptr, const char **endptr, int base, > { > char *ep; > > + assert((unsigned) base <= 36 && base != 1); > if (!nptr) { > if (endptr) { > *endptr = nptr; > @@ -502,6 +507,7 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base, > { > char *ep; > > + assert((unsigned) base <= 36 && base != 1); > if (!nptr) { > if (endptr) { > *endptr = nptr; > @@ -525,6 +531,7 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base, > { > char *ep; > > + assert((unsigned) base <= 36 && base != 1); > if (!nptr) { > if (endptr) { > *endptr = nptr; > @@ -657,6 +664,7 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr, > char *endp = (char *)s; > unsigned long long val = 0; > > + assert((unsigned) base <= 36 && base != 1); > if (!s) { > r = -EINVAL; > goto out; > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Le 06/12/2018 à 16:18, Eric Blake a écrit : > POSIX states that the value of endptr is unspecified if strtol() > fails with EINVAL due to an invalid base argument. Since none of > the callers to check_strtox_error() initialized endptr, we could > end up propagating uninitialized data back to a caller on error. > However, passing an out-of-range base is already a sign of poor > programming, so let's just assert that base is in range, at which > point check_strtox_error() can be tightened to assert that it is > receiving an initialized ep that points somewhere within the > caller's original string, regardless of whether strto*() succeeded > or failed with ERANGE. > > Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > Also tested that this does not negatively impact David's pending > additions of qemu_strtod{,_finite}(). Thus: > Based-on: <20181121164421.20780-1-david@redhat.com> > > util/cutils.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > Applied to my trivial-patches branch. Thanks, Laurent
Patch
diff --git a/util/cutils.c b/util/cutils.c index 91930d1bbeb..e098debdc0c 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -278,6 +278,7 @@ int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result) static int check_strtox_error(const char *nptr, char *ep, const char **endptr, int libc_errno) { + assert(ep >= nptr); if (endptr) { *endptr = ep; } @@ -325,6 +326,7 @@ int qemu_strtoi(const char *nptr, const char **endptr, int base, char *ep; long long lresult; + assert((unsigned) base <= 36 && base != 1); if (!nptr) { if (endptr) { *endptr = nptr; @@ -377,6 +379,7 @@ int qemu_strtoui(const char *nptr, const char **endptr, int base, char *ep; long long lresult; + assert((unsigned) base <= 36 && base != 1); if (!nptr) { if (endptr) { *endptr = nptr; @@ -433,6 +436,7 @@ int qemu_strtol(const char *nptr, const char **endptr, int base, { char *ep; + assert((unsigned) base <= 36 && base != 1); if (!nptr) { if (endptr) { *endptr = nptr; @@ -475,6 +479,7 @@ int qemu_strtoul(const char *nptr, const char **endptr, int base, { char *ep; + assert((unsigned) base <= 36 && base != 1); if (!nptr) { if (endptr) { *endptr = nptr; @@ -502,6 +507,7 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base, { char *ep; + assert((unsigned) base <= 36 && base != 1); if (!nptr) { if (endptr) { *endptr = nptr; @@ -525,6 +531,7 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base, { char *ep; + assert((unsigned) base <= 36 && base != 1); if (!nptr) { if (endptr) { *endptr = nptr; @@ -657,6 +664,7 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr, char *endp = (char *)s; unsigned long long val = 0; + assert((unsigned) base <= 36 && base != 1); if (!s) { r = -EINVAL; goto out;
POSIX states that the value of endptr is unspecified if strtol() fails with EINVAL due to an invalid base argument. Since none of the callers to check_strtox_error() initialized endptr, we could end up propagating uninitialized data back to a caller on error. However, passing an out-of-range base is already a sign of poor programming, so let's just assert that base is in range, at which point check_strtox_error() can be tightened to assert that it is receiving an initialized ep that points somewhere within the caller's original string, regardless of whether strto*() succeeded or failed with ERANGE. Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- Also tested that this does not negatively impact David's pending additions of qemu_strtod{,_finite}(). Thus: Based-on: <20181121164421.20780-1-david@redhat.com> util/cutils.c | 8 ++++++++ 1 file changed, 8 insertions(+)