Date: Tue, 10 Dec 2002 13:22:14 +0000
From: "Richard Dawe" <rich AT phekda DOT freeserve DOT co DOT uk>
Sender: rich AT phekda DOT freeserve DOT co DOT uk
To: djgpp-workers AT delorie DOT com
X-Mailer: Emacs 21.3.50 (via feedmail 8.3.emacs20_6 I) and Blat ver 1.8.6
Subject: scanf buffer overflow; support 'hh' conversion specifier [PATCH]
Message-Id: <E18LkKs-0000qc-00@phekda.freeserve.co.uk>
Reply-To: djgpp-workers AT delorie DOT com

Hello.

Below is a patch to:

* fix a scanf buffer overflow with the 'X' format specifier;
* add support for the 'hh' (char) conversion specifier.

The program below, bigx-bug.c, will assert, if built against 2.03
or current CVS. There are two shorts that will be adjacent in memory:
s_previous and s_res. When scanning in a number that is larger than
a short, s_previous will be trashed, rather than the number
being truncated and being stored in s_res only.

The code that causes the buffer overflow is the special handling
for capitalised letters (for compatibility with Borland C) - D, I, U
and O. For some reason X was included in this too. I think the code
mistakenly sets 'size' to LONG for X. I think it should leave it alone
and let 'size' be set by the normal mechanisms - 'h', 'l', 'll', etc.

Here's what bigx-bug displays on my system, when built
against a CVS tree with the patch below applied:

DJGPP 2.04
Got 0x0, expected 0x0
Got 0x0, expected 0x0
s_previous == 0x0, expected 0x0
Got 0x0, expected 0x0
Got 0x0, expected 0x0
Got 0xffffffffffffff00
Got 0xffffffffffff0000
Got 0xffffffff00000000
Got 0xffffffff00000000

The last three lines are correct - because the result is stored
in a long long variable, only the bottom 8 (hh), 16 (h), 32 ("regular")
or 32 (l) bits.

I've also added support for the 'hh' (char) conversion specifier
to scanf and a test program for 'hh'.

OK to commit? (On Sunday?)

Bye, Rich =]

---Start bigx-bug.c---
#include <assert.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/version.h>

int
main (void)
{
  signed char sc_res;

#define S_PREVIOUS_INITIAL 0
  short s_previous = S_PREVIOUS_INITIAL;
  short s_res;

  int i_res;

  long l_res;

  long long res;

  int ret;

  printf("DJGPP %d.%02d\n", __DJGPP__, __DJGPP_MINOR__);

  /* - Normal, correct-sized variables. - */
#if ((__DJGPP__ == 2) && (__DJGPP_MINOR__ > 3)) || (__DJGPP__ > 2)
  /* char */
  sc_res = 0xff;
  ret = sscanf("0x100", "%hhX", &sc_res);
  assert(ret > 0);
  printf("Got 0x%x, expected 0x%x\n", sc_res, 0);
#endif

  /* short */
  s_res = 0xffff;
  ret = sscanf("0x10000", "%hX", &s_res);
  assert(ret > 0);
  printf("Got 0x%x, expected 0x%x\n", s_res, 0);

  printf("s_previous == 0x%x, expected 0x%x\n",
	 s_previous, S_PREVIOUS_INITIAL);
  assert(s_previous == S_PREVIOUS_INITIAL);

  /* int */
  i_res = 0xffffffffU;
  ret = sscanf("0x100000000", "%X", &i_res);
  assert(ret > 0);
  printf("Got 0x%x, expected 0x%x\n", i_res, 0);

  /* long */
  l_res = 0xffffffffUL;
  ret = sscanf("0x100000000", "%lX", &l_res);
  assert(ret > 0);
  printf("Got 0x%lx, expected 0x%x\n", l_res, 0);

  /* - In a long long, to check for buffer-overflow. - */
#if ((__DJGPP__ == 2) && (__DJGPP_MINOR__ > 3)) || (__DJGPP__ > 2)
  /* char */
  res = 0xffffffffffffffffULL;
  ret = sscanf("0x100", "%hhX", &res);
  assert(ret > 0);
  printf("Got 0x%llx\n", res);
#endif

  /* short */
  res = 0xffffffffffffffffULL;
  ret = sscanf("0x10000", "%hX", &res);
  assert(ret > 0);
  printf("Got 0x%llx\n", res);

  /* int */
  res = 0xffffffffffffffffULL;
  ret = sscanf("0x100000000", "%X", &res);
  assert(ret > 0);
  printf("Got 0x%llx\n", res);

  /* long */
  res = 0xffffffffffffffffULL;
  ret = sscanf("0x100000000", "%lX", &res);
  assert(ret > 0);
  printf("Got 0x%llx\n", res);

  return(EXIT_SUCCESS);
}
---End bigx-bug.c---

Index: src/libc/ansi/stdio/doscan.c
===================================================================
RCS file: /cvs/djgpp/djgpp/src/libc/ansi/stdio/doscan.c,v
retrieving revision 1.11
diff -p -c -3 -r1.11 doscan.c
*** src/libc/ansi/stdio/doscan.c	14 Jun 2002 14:19:42 -0000	1.11
--- src/libc/ansi/stdio/doscan.c	10 Dec 2002 13:07:37 -0000
***************
*** 13,22 ****
  #define	SPC	01
  #define	STP	02
  
! #define	SHORT	0
! #define	REGULAR	1
! #define	LONG	2
! #define LONGDOUBLE 4
  #define	INT	0
  #define	FLOAT	1
  
--- 13,24 ----
  #define	SPC	01
  #define	STP	02
  
! #define CHAR	0
! #define	SHORT	1
! #define	REGULAR	2
! #define	LONG	4
! #define LONGDOUBLE 8
! 
  #define	INT	0
  #define	FLOAT	1
  
*************** _doscan_low(FILE *iop, int (*scan_getc)(
*** 91,96 ****
--- 93,103 ----
      else if (ch=='h') {
        size = SHORT;
        ch = *fmt++;
+       if (ch=='h')
+       {
+ 	size = CHAR;
+ 	ch = *fmt++;
+       }
      } else if (ch=='L') {
        size = LONGDOUBLE;
        ch = *fmt++;
*************** _doscan_low(FILE *iop, int (*scan_getc)(
*** 107,116 ****
  	 This extension is supported by some compilers (e.g. Borland C).
  	 Old pre-ANSI compilers, such as Turbo C 2.0, also interpreted
  	 %E, %F and %G to store in a double (rather than a float), but
! 	 this contradicts the ANSI Standard, so we don't support it.  */
!       if (ch == 'd' || ch == 'i' || ch == 'o' || ch == 'u' || ch == 'x')
        {
! 	if (size==LONG && ch != 'x') /* ANSI: %lX is long, not long long */
  	  size = LONGDOUBLE;
  	else if (size != LONGDOUBLE)
  	  size = LONG;
--- 114,125 ----
  	 This extension is supported by some compilers (e.g. Borland C).
  	 Old pre-ANSI compilers, such as Turbo C 2.0, also interpreted
  	 %E, %F and %G to store in a double (rather than a float), but
! 	 this contradicts the ANSI Standard, so we don't support it.
!          %X should be treated as per the ANSI Standard - no length
! 	 is implied by the upper-case x. */
!       if (ch == 'd' || ch == 'i' || ch == 'o' || ch == 'u')
        {
! 	if (size == LONG)
  	  size = LONGDOUBLE;
  	else if (size != LONGDOUBLE)
  	  size = LONG;
*************** _doscan_low(FILE *iop, int (*scan_getc)(
*** 125,130 ****
--- 134,141 ----
          break;
        if (size==LONG)
  	*(long*)ptr = nchars;
+       else if (size==CHAR)
+ 	*(char*)ptr = nchars;
        else if (size==SHORT)
          *(short*)ptr = nchars;
        else if (size==LONGDOUBLE)
*************** _innum(int *ptr, int type, int len, int 
*** 292,297 ****
--- 303,312 ----
  
    case (FLOAT<<4) | LONGDOUBLE:
      *(long double *)ptr = _atold(numbuf);
+     break;
+ 
+   case (INT<<4) | CHAR:
+     *(char *)ptr = (char)lcval;
      break;
  
    case (INT<<4) | SHORT:
Index: src/docs/kb/wc204.txi
===================================================================
RCS file: /cvs/djgpp/djgpp/src/docs/kb/wc204.txi,v
retrieving revision 1.125
diff -p -c -3 -r1.125 wc204.txi
*** src/docs/kb/wc204.txi	6 Dec 2002 09:36:35 -0000	1.125
--- src/docs/kb/wc204.txi	10 Dec 2002 13:07:46 -0000
*************** The function @code{strtold} was added.
*** 794,798 ****
  
  @findex _doprnt AT r{, and }hh AT r{ conversion qualifier}
  @findex printf AT r{, and }hh AT r{ conversion qualifier}
! The @code{hh} conversion qualifier is now supported by @code{_doprnt}
! and the @code{printf} family of functions.
--- 794,801 ----
  
  @findex _doprnt AT r{, and }hh AT r{ conversion qualifier}
  @findex printf AT r{, and }hh AT r{ conversion qualifier}
! @findex _doscan AT r{, and }hh AT r{ conversion qualifier}
! @findex scanf AT r{, and }hh AT r{ conversion qualifier}
! The @code{hh} conversion qualifier is now supported by @code{_doprnt},
! @code{_doscan}, the @code{printf} family of functions
! and the @code{scanf} family of functions.
Index: tests/libc/ansi/stdio/makefile
===================================================================
RCS file: /cvs/djgpp/djgpp/tests/libc/ansi/stdio/makefile,v
retrieving revision 1.7
diff -p -c -3 -r1.7 makefile
*** tests/libc/ansi/stdio/makefile	6 Dec 2002 09:37:53 -0000	1.7
--- tests/libc/ansi/stdio/makefile	10 Dec 2002 13:07:46 -0000
*************** SRC += mktemp.c
*** 16,21 ****
--- 16,22 ----
  SRC += printf.c
  SRC += printf2.c
  SRC += sscanf.c
+ SRC += sscanf2.c
  SRC += tmpnam.c
  SRC += tscanf.c
  SRC += tsnprtf.c
*** /dev/null	Tue Dec 10 13:08:47 2002
--- tests/libc/ansi/stdio/sscanf2.c	Tue Dec 10 11:54:54 2002
***************
*** 0 ****
--- 1,117 ----
+ #include <assert.h>
+ #include <stdio.h>
+ #include <stdlib.h>
+ #include <string.h>
+ 
+ typedef struct {
+   const char *str;
+   const char *fmt;
+   const signed char res;
+ } signed_testcase_t;
+ 
+ typedef struct {
+   const char *str;
+   const char *fmt;
+   const unsigned char res;
+ } unsigned_testcase_t;
+ 
+ signed_testcase_t s_testcases[] = {
+   /* Normal */
+   { "127",  "%hhd", 127 },
+   { "127",  "%hhi", 127 },
+   { "0x7f", "%hhx", 127 },
+   { "0x7F", "%hhX", 127 },
+   { "0x7f", "%hhx", 127 },
+   { "0x7F", "%hhX", 127 },
+   { "0177", "%hho", 127 },
+ 
+   /* Truncation */
+   { "255",     "%hhd", -1 },
+   { "256",     "%hhd",  0 },
+   { "0xff",    "%hhx", -1 },
+   { "0x100",   "%hhx",  0 },
+   { "0377",    "%hho", -1 },
+   { "0400",    "%hho",  0 },
+   { "65535",   "%hhd", -1 },
+   { "65536",   "%hhd",  0 },
+   { "0xffff",  "%hhx", -1 },
+   { "0x10000", "%hhx",  0 },
+   { "177777",  "%hho", -1 },
+   { "200000",  "%hho",  0 },
+ 
+   /* Terminator */
+   { NULL, NULL, 0 }
+ };
+ 
+ unsigned_testcase_t u_testcases[] = {
+   /* Normal */
+   { "255", "%hhu", 255 },
+ 
+   /* Truncation */
+   { "256",   "%hhu",   0 },
+   { "65535", "%hhu", 255 },
+   { "65536", "%hhu",   0 },
+ 
+   /* Terminator */
+   { NULL, NULL, 0 }
+ };
+ 
+ static void
+ fail (const int testnum,
+       const char *input,
+       const char *fmt,
+       const char *reason,
+       const long code)
+ {
+   fprintf(stderr,
+ 	  "FAIL: Test %d: %s %s: %s: %ld\n",
+ 	  testnum, input, fmt, reason, code);
+   exit(EXIT_FAILURE);
+ }
+ 
+ int
+ main (void)
+ {
+   signed char   s_res;
+   unsigned char u_res;
+   int           testnum = 0;
+   int           ret;
+   int           i;
+ 
+   /* Signed testcases */
+   for (i = 0; s_testcases[i].str != NULL; i++) {
+     testnum++;
+     s_res = 0;
+ 
+     ret = sscanf(s_testcases[i].str, s_testcases[i].fmt, &s_res);
+     if ((ret == EOF) || (ret < 1)) {
+       fail(testnum, s_testcases[i].str, s_testcases[i].fmt,
+ 	   "sscanf failed", ret);
+     }
+ 
+     if (s_testcases[i].res != s_res) {
+       fail(testnum, s_testcases[i].str, s_testcases[i].fmt,
+ 	   "unexpected result", s_res);
+     }
+   }
+ 
+   /* Unsigned testcases */
+   for (i = 0; u_testcases[i].str != NULL; i++) {
+     testnum++;
+     u_res = 0;
+ 
+     ret = sscanf(u_testcases[i].str, u_testcases[i].fmt, &u_res);
+     if ((ret == EOF) || (ret < 1)) {
+       fail(testnum, u_testcases[i].str, u_testcases[i].fmt,
+ 	   "sscanf failed", ret);
+     }
+ 
+     if (u_testcases[i].res != u_res) {
+       fail(testnum, u_testcases[i].str, u_testcases[i].fmt,
+ 	   "unexpected result", u_res);
+     }
+   }
+ 
+   puts("PASS");
+   return(EXIT_SUCCESS);
+ }