This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: vfprintf typing problem


On Fri, Mar 30, 2012 at 4:18 AM, David Miller <davem@davemloft.net> wrote:
> I propose the following patch.
>
> At the points where the "(size_t) -1 / sizeof(CHAT_T)" checks exist
> the 'width' has been "normalized" to be positive. ?If it was negative,
> that state has been recorded in the 'left' variable.
>
> So we can just directly sanity check whether width (plus the 32-bytes
> of slack) is >= INT_MAX.
>
> Therefore we have two levels of protection in this code:
>
> 1) Individual field widths are validated against INT_MAX
>
> 2) The total result (stored in 'done') is validated against
> ? overflowing INT_MAX as it accumulates, via the done_add() macro.
>
> I've also adjusted bug22.c so that it tests both of those cases.
>
> 2012-03-30 ?David S. Miller ?<davem@davemloft.net>
>
> ? ? ? ?* stdio-common/vfprintf.c (vfprintf): Validate width against
> ? ? ? ?overflow of INT_MAX. ?Set errno to EOVERFLOW when 'done' overflows
> ? ? ? ?INT_MAX.
> ? ? ? ?* stdio-common/bug22.c: Adjust to test both width INT_MAX overflow
> ? ? ? ?as well as total length INT_MAX overflow. ?Check explicitly for
> ? ? ? ?proper errno values.
>
> diff --git a/stdio-common/bug22.c b/stdio-common/bug22.c
> index 2228388..6b6bb43 100644
> --- a/stdio-common/bug22.c
> +++ b/stdio-common/bug22.c
> @@ -1,12 +1,18 @@
> ?/* BZ #5424 */
> ?#include <stdio.h>
> +#include <errno.h>
>
> +/* INT_MAX + 1 */
> ?#define N 2147483648
>
> +/* (INT_MAX / 2) + 2 */
> +#define N2 1073741825
> +
> ?#define STRINGIFY(S) #S
> ?#define MAKE_STR(S) STRINGIFY(S)
>
> ?#define SN MAKE_STR(N)
> +#define SN2 MAKE_STR(N2)
>
> ?static int
> ?do_test (void)
> @@ -20,11 +26,15 @@ do_test (void)
> ? ? ? return 1;
> ? ? }
>
> - ?ret = fprintf (fp, "%" SN "d%" SN "d", 1, 1);
> + ?ret = fprintf (fp, "%" SN "d", 1);
> + ?printf ("ret = %d\n", ret);
> + ?if (ret != -1 || errno != ERANGE)
> + ? ? ? ? return 1;
>
> + ?ret = fprintf (fp, "%" SN2 "d%" SN2 "d", 1, 1);
> ? printf ("ret = %d\n", ret);
>
> - ?return ret != -1;
> + ?return ret != -1 || errno != EOVERFLOW;
> ?}
>
> ?#define TIMEOUT 30
> diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
> index 1e90483..4875fe0 100644
> --- a/stdio-common/vfprintf.c
> +++ b/stdio-common/vfprintf.c
> @@ -71,6 +71,7 @@
> ? ? ? ? ? ? ? ? ? ? ? ? ?< _val, 0)) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? ? { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? ?done = -1; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? ?__set_errno (EOVERFLOW); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? ? ?goto all_done; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? } ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? done += _val; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> @@ -141,12 +142,18 @@
> ? do ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? { ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? assert ((size_t) done <= (size_t) INT_MAX); ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> - ? ? ?if ((size_t) PUT (s, (String), (Len)) != (size_t) (Len) ? ? ? ? ? ? ? ?\
> - ? ? ? ? || (size_t) INT_MAX - (size_t) done < (size_t) (Len)) ? ? ? ? ? ? ? \
> + ? ? ?if ((size_t) PUT (s, (String), (Len)) != (size_t) (Len)) ? ? ? ? ? ? ? \
> ? ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? ? ? ?done = -1; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? ? ?goto all_done; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? ?} ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ?if (__builtin_expect ((unsigned int) INT_MAX - (unsigned int) done ? ? ? \
> + ? ? ? ? ? ? ? ? ? ? ? ? < (Len), 0)) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ?{ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? done = -1; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ? ?__set_errno (EOVERFLOW); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> + ? ? ? goto all_done; ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> + ? ? ?} ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? ? ? done += (Len); ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
> ? ? } ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
> ? while (0)
> @@ -1449,7 +1456,8 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
> ? ? ? ? ? ?left = 1;
> ? ? ? ? ?}
>
> - ? ? ? if (__builtin_expect (width >= (size_t) -1 / sizeof (CHAR_T) - 32, 0))
> + ? ? ? if (__builtin_expect ((unsigned int) width >= INT_MAX
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? / sizeof (CHAR_T) - 32, 0))
> ? ? ? ? ?{
> ? ? ? ? ? ?__set_errno (ERANGE);
> ? ? ? ? ? ?done = -1;
> @@ -1481,7 +1489,7 @@ vfprintf (FILE *s, const CHAR_T *format, va_list ap)
> ? ? LABEL (width):
> ? ? ? width = read_int (&f);
>
> - ? ? ?if (__builtin_expect (width >= (size_t) -1 / sizeof (CHAR_T) - 32, 0))
> + ? ? ?if (__builtin_expect ((unsigned int) width >= INT_MAX / sizeof (CHAR_T) - 32, 0))
> ? ? ? ?{
> ? ? ? ? ?__set_errno (ERANGE);
> ? ? ? ? ?done = -1;

Thanks for slogging through this, the patch is looking good so far.

Could you explain why we don't also need to fix the same problem with `prec'?

Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]