Subject: bug#8719: closed (fixes committed to trunk)

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 27 May 2011 20:41:02 +0000

Your bug report

#8719: ccl: add some integer overflow checks

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
8719: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8719
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: fixes committed to trunk
From: Paul Eggert <[email-masked]>
To: [email-masked], [email-masked], [email-masked]
Date: Fri, 27 May 2011 13:40:10 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

I just committed bzr 104390 to the Emacs trunk.
It merges fixes for bugs 8668, 8719, and 8722 as
previously discussed.

For Bug#8719, although the patch should fix the problems
it may not be the best patch.  Kenichi Handa wrote that
he'll check it.  Kenichi, if there are problems with it,
please feel free to replace it with something better
or to send me email with suggestions and I'll work on
making it better.  Thanks.

Subject: ccl: add some integer overflow checks
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Sun, 22 May 2011 23:53:27 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

I did a quick pass through ccl.c and found several places where
integer overflow could cause problems.  Here's a proposed fix for some
of the problems.

Does anybody have a good suggestion for testing these?  I'm no
expert in CCL.

ccl: add integer overflow checks
* ccl.c (CCL_CODE_MAX, GET_CCL_RANGE, GET_CCL_CODE, GET_CCL_INT):
(IN_INT_RANGE): New macros.
(ccl_driver): Use them to check for integer overflow when
decoding a CCL program.  Many of the new checks are whether XINT (x)
fits in int; it doesn't always, on 64-bit hosts.  The new version
doesn't catch all possible integer overflows, but it's an
improvement.
=== modified file 'src/ccl.c'
--- src/ccl.c	2011-05-12 07:07:06 +0000
+++ src/ccl.c	2011-05-23 06:46:58 +0000
@@ -98,6 +98,8 @@
    and `rrr' are CCL register number, `XXXXX' is one of the following
    CCL commands.  */

+#define CCL_CODE_MAX ((1 << (28 - 1)) - 1)
+
 /* CCL commands

    Each comment fields shows one or more lines for command syntax and
@@ -742,6 +744,24 @@

 #endif

+#define GET_CCL_RANGE(var, ccl_prog, ic, lo, hi)		\
+  do								\
+    {								\
+      EMACS_INT prog_word = XINT ((ccl_prog)[ic]);		\
+      if (! ((lo) <= prog_word && prog_word <= (hi)))		\
+	CCL_INVALID_CMD;					\
+      (var) = prog_word;					\
+    }								\
+  while (0)
+
+#define GET_CCL_CODE(code, ccl_prog, ic)			\
+  GET_CCL_RANGE (code, ccl_prog, ic, 0, CCL_CODE_MAX)
+
+#define GET_CCL_INT(var, ccl_prog, ic)				\
+  GET_CCL_RANGE (var, ccl_prog, ic, INT_MIN, INT_MAX)
+
+#define IN_INT_RANGE(val) (INT_MIN <= (val) && (val) <= INT_MAX)
+
 /* Encode one character CH to multibyte form and write to the current
    output buffer.  If CH is less than 256, CH is written as is.  */
 #define CCL_WRITE_CHAR(ch)			\
@@ -899,7 +919,7 @@
 	}

       this_ic = ic;
-      code = XINT (ccl_prog[ic]); ic++;
+      GET_CCL_CODE (code, ccl_prog, ic++);
       field1 = code >> 8;
       field2 = (code & 0xFF) >> 5;

@@ -920,15 +940,14 @@
 	  break;

 	case CCL_SetConst:	/* 00000000000000000000rrrXXXXX */
-	  reg[rrr] = XINT (ccl_prog[ic]);
-	  ic++;
+	  GET_CCL_INT (reg[rrr], ccl_prog, ic++);
 	  break;

 	case CCL_SetArray:	/* CCCCCCCCCCCCCCCCCCCCRRRrrrXXXXX */
 	  i = reg[RRR];
 	  j = field1 >> 3;
 	  if ((unsigned int) i < j)
-	    reg[rrr] = XINT (ccl_prog[ic + i]);
+	    GET_CCL_INT (reg[rrr], ccl_prog, ic + i);
 	  ic += j;
 	  break;

@@ -956,13 +975,13 @@
 	  break;

 	case CCL_WriteConstJump: /* A--D--D--R--E--S--S-000XXXXX */
-	  i = XINT (ccl_prog[ic]);
+	  GET_CCL_INT (i, ccl_prog, ic);
 	  CCL_WRITE_CHAR (i);
 	  ic += ADDR;
 	  break;

 	case CCL_WriteConstReadJump: /* A--D--D--R--E--S--S-rrrXXXXX */
-	  i = XINT (ccl_prog[ic]);
+	  GET_CCL_INT (i, ccl_prog, ic);
 	  CCL_WRITE_CHAR (i);
 	  ic++;
 	  CCL_READ_CHAR (reg[rrr]);
@@ -970,18 +989,17 @@
 	  break;

 	case CCL_WriteStringJump: /* A--D--D--R--E--S--S-000XXXXX */
-	  j = XINT (ccl_prog[ic]);
-	  ic++;
+	  GET_CCL_INT (j, ccl_prog, ic++);
 	  CCL_WRITE_STRING (j);
 	  ic += ADDR - 1;
 	  break;

 	case CCL_WriteArrayReadJump: /* A--D--D--R--E--S--S-rrrXXXXX */
 	  i = reg[rrr];
-	  j = XINT (ccl_prog[ic]);
+	  GET_CCL_INT (j, ccl_prog, ic);
 	  if ((unsigned int) i < j)
 	    {
-	      i = XINT (ccl_prog[ic + 1 + i]);
+	      GET_CCL_INT (i, ccl_prog, ic + 1 + i);
 	      CCL_WRITE_CHAR (i);
 	    }
 	  ic += j + 2;
@@ -998,10 +1016,14 @@
 	  CCL_READ_CHAR (reg[rrr]);
 	  /* fall through ... */
 	case CCL_Branch:	/* CCCCCCCCCCCCCCCCCCCCrrrXXXXX */
-	  if ((unsigned int) reg[rrr] < field1)
-	    ic += XINT (ccl_prog[ic + reg[rrr]]);
-	  else
-	    ic += XINT (ccl_prog[ic + field1]);
+	{
+	  int incr;
+	  GET_CCL_INT (incr, ccl_prog,
+		       ic + ((unsigned int) reg[rrr] < field1
+			     ? reg[rrr]
+			     : field1));
+	  ic += incr;
+	}
 	  break;

 	case CCL_ReadRegister:	/* CCCCCCCCCCCCCCCCCCCCrrXXXXX */
@@ -1009,7 +1031,7 @@
 	    {
 	      CCL_READ_CHAR (reg[rrr]);
 	      if (!field1) break;
-	      code = XINT (ccl_prog[ic]); ic++;
+	      GET_CCL_CODE (code, ccl_prog, ic++);
 	      field1 = code >> 8;
 	      field2 = (code & 0xFF) >> 5;
 	    }
@@ -1018,7 +1040,7 @@
 	case CCL_WriteExprConst:  /* 1:00000OPERATION000RRR000XXXXX */
 	  rrr = 7;
 	  i = reg[RRR];
-	  j = XINT (ccl_prog[ic]);
+	  GET_CCL_INT (j, ccl_prog, ic);
 	  op = field1 >> 6;
 	  jump_address = ic + 1;
 	  goto ccl_set_expr;
@@ -1029,7 +1051,7 @@
 	      i = reg[rrr];
 	      CCL_WRITE_CHAR (i);
 	      if (!field1) break;
-	      code = XINT (ccl_prog[ic]); ic++;
+	      GET_CCL_CODE (code, ccl_prog, ic++);
 	      field1 = code >> 8;
 	      field2 = (code & 0xFF) >> 5;
 	    }
@@ -1051,10 +1073,7 @@
 	    /* If FFF is nonzero, the CCL program ID is in the
                following code.  */
 	    if (rrr)
-	      {
-		prog_id = XINT (ccl_prog[ic]);
-		ic++;
-	      }
+	      GET_CCL_INT (prog_id, ccl_prog, ic++);
 	    else
 	      prog_id = field1;

@@ -1097,7 +1116,7 @@
 	  i = reg[rrr];
 	  if ((unsigned int) i < field1)
 	    {
-	      j = XINT (ccl_prog[ic + i]);
+	      GET_CCL_INT (j, ccl_prog, ic + i);
 	      CCL_WRITE_CHAR (j);
 	    }
 	  ic += field1;
@@ -1122,8 +1141,7 @@
 	  CCL_SUCCESS;

 	case CCL_ExprSelfConst: /* 00000OPERATION000000rrrXXXXX */
-	  i = XINT (ccl_prog[ic]);
-	  ic++;
+	  GET_CCL_INT (i, ccl_prog, ic++);
 	  op = field1 >> 6;
 	  goto ccl_expr_self;

@@ -1159,9 +1177,9 @@

 	case CCL_SetExprConst:	/* 00000OPERATION000RRRrrrXXXXX */
 	  i = reg[RRR];
-	  j = XINT (ccl_prog[ic]);
+	  GET_CCL_INT (j, ccl_prog, ic++);
 	  op = field1 >> 6;
-	  jump_address = ++ic;
+	  jump_address = ic;
 	  goto ccl_set_expr;

 	case CCL_SetExprReg:	/* 00000OPERATIONRrrRRRrrrXXXXX */
@@ -1175,10 +1193,9 @@
 	  CCL_READ_CHAR (reg[rrr]);
 	case CCL_JumpCondExprConst: /* A--D--D--R--E--S--S-rrrXXXXX */
 	  i = reg[rrr];
-	  op = XINT (ccl_prog[ic]);
-	  jump_address = ic++ + ADDR;
-	  j = XINT (ccl_prog[ic]);
-	  ic++;
+	  jump_address = ic + ADDR;
+	  GET_CCL_INT (op, ccl_prog, ic++);
+	  GET_CCL_INT (j, ccl_prog, ic++);
 	  rrr = 7;
 	  goto ccl_set_expr;

@@ -1186,10 +1203,10 @@
 	  CCL_READ_CHAR (reg[rrr]);
 	case CCL_JumpCondExprReg:
 	  i = reg[rrr];
-	  op = XINT (ccl_prog[ic]);
-	  jump_address = ic++ + ADDR;
-	  j = reg[XINT (ccl_prog[ic])];
-	  ic++;
+	  jump_address = ic + ADDR;
+	  GET_CCL_INT (op, ccl_prog, ic++);
+	  GET_CCL_RANGE (j, ccl_prog, ic++, 0, 7);
+	  j = reg[j];
 	  rrr = 7;

 	ccl_set_expr:
@@ -1267,18 +1284,27 @@
 	      break;

 	    case CCL_TranslateCharacterConstTbl:
-	      op = XINT (ccl_prog[ic]); /* table */
-	      ic++;
-	      i = CCL_DECODE_CHAR (reg[RRR], reg[rrr]);
-	      op = translate_char (GET_TRANSLATION_TABLE (op), i);
-	      CCL_ENCODE_CHAR (op, charset_list, reg[RRR], reg[rrr]);
+	      {
+		EMACS_INT eop;
+		GET_CCL_RANGE (eop, ccl_prog, ic++, 0,
+			       (VECTORP (Vtranslation_table_vector)
+				? ASIZE (Vtranslation_table_vector)
+				: -1));
+		i = CCL_DECODE_CHAR (reg[RRR], reg[rrr]);
+		op = translate_char (GET_TRANSLATION_TABLE (eop), i);
+		CCL_ENCODE_CHAR (op, charset_list, reg[RRR], reg[rrr]);
+	      }
 	      break;

 	    case CCL_LookupIntConstTbl:
-	      op = XINT (ccl_prog[ic]); /* table */
-	      ic++;
 	      {
-		struct Lisp_Hash_Table *h = GET_HASH_TABLE (op);
+		EMACS_INT eop;
+		struct Lisp_Hash_Table *h;
+		GET_CCL_RANGE (eop, ccl_prog, ic++, 0,
+			       (VECTORP (Vtranslation_hash_table_vector)
+				? ASIZE (Vtranslation_hash_table_vector)
+				: -1));
+		h = GET_HASH_TABLE (eop);

 		op = hash_lookup (h, make_number (reg[RRR]), NULL);
 		if (op >= 0)
@@ -1297,18 +1323,22 @@
 	      break;

 	    case CCL_LookupCharConstTbl:
-	      op = XINT (ccl_prog[ic]); /* table */
-	      ic++;
-	      i = CCL_DECODE_CHAR (reg[RRR], reg[rrr]);
 	      {
-		struct Lisp_Hash_Table *h = GET_HASH_TABLE (op);
+		EMACS_INT eop;
+		struct Lisp_Hash_Table *h;
+		GET_CCL_RANGE (eop, ccl_prog, ic++, 0,
+			       (VECTORP (Vtranslation_hash_table_vector)
+				? ASIZE (Vtranslation_hash_table_vector)
+				: -1));
+		i = CCL_DECODE_CHAR (reg[RRR], reg[rrr]);
+		h = GET_HASH_TABLE (eop);

 		op = hash_lookup (h, make_number (i), NULL);
 		if (op >= 0)
 		  {
 		    Lisp_Object opl;
 		    opl = HASH_VALUE (h, op);
-		    if (!INTEGERP (opl))
+		    if (! (INTEGERP (opl) && IN_INT_RANGE (XINT (opl))))
 		      CCL_INVALID_CMD;
 		    reg[RRR] = XINT (opl);
 		    reg[7] = 1; /* r7 true for success */
@@ -1321,9 +1351,10 @@
 	    case CCL_IterateMultipleMap:
 	      {
 		Lisp_Object map, content, attrib, value;
-		int point, size, fin_ic;
+		EMACS_INT point, size;
+		int fin_ic;

-		j = XINT (ccl_prog[ic++]); /* number of maps. */
+		GET_CCL_INT (j, ccl_prog, ic++); /* number of maps. */
 		fin_ic = ic + j;
 		op = reg[rrr];
 		if ((j > reg[RRR]) && (j >= 0))
@@ -1343,7 +1374,7 @@

 		    size = ASIZE (Vcode_conversion_map_vector);
 		    point = XINT (ccl_prog[ic++]);
-		    if (point >= size) continue;
+		    if (! (0 <= point && point < size)) continue;
 		    map = AREF (Vcode_conversion_map_vector, point);

 		    /* Check map validity.  */
@@ -1358,18 +1389,19 @@
 		    /* check map type,
 		       [STARTPOINT VAL1 VAL2 ...] or
 		       [t ELEMENT STARTPOINT ENDPOINT]  */
-		    if (NUMBERP (content))
+		    if (INTEGERP (content))
 		      {
-			point = XUINT (content);
-			point = op - point + 1;
-			if (!((point >= 1) && (point < size))) continue;
-			content = AREF (map, point);
+			point = XINT (content);
+			if (!(point <= op && op - point + 1 < size)) continue;
+			content = AREF (map, op - point + 1);
 		      }
 		    else if (EQ (content, Qt))
 		      {
 			if (size != 4) continue;
-			if ((op >= XUINT (AREF (map, 2)))
-			    && (op < XUINT (AREF (map, 3))))
+			if (INTEGERP (AREF (map, 2))
+			    && XINT (AREF (map, 2)) <= op
+			    && INTEGERP (AREF (map, 3))
+			    && op < XINT (AREF (map, 3)))
 			  content = AREF (map, 1);
 			else
 			  continue;
@@ -1379,7 +1411,7 @@

 		    if (NILP (content))
 		      continue;
-		    else if (NUMBERP (content))
+		    else if (INTEGERP (content) && IN_INT_RANGE (XINT (content)))
 		      {
 			reg[RRR] = i;
 			reg[rrr] = XINT(content);
@@ -1394,10 +1426,11 @@
 		      {
 			attrib = XCAR (content);
 			value = XCDR (content);
-			if (!NUMBERP (attrib) || !NUMBERP (value))
+			if (! (INTEGERP (attrib) && INTEGERP (value)
+			       && IN_INT_RANGE (XINT (value))))
 			  continue;
 			reg[RRR] = i;
-			reg[rrr] = XUINT (value);
+			reg[rrr] = XINT (value);
 			break;
 		      }
 		    else if (SYMBOLP (content))
@@ -1432,8 +1465,9 @@
 		  mapping_stack_pointer = mapping_stack;
 		stack_idx_of_map_multiple = 0;

-		map_set_rest_length =
-		  XINT (ccl_prog[ic++]); /* number of maps and separators. */
+		/* Get number of maps and separators.  */
+		GET_CCL_INT (map_set_rest_length, ccl_prog, ic++);
+
 		fin_ic = ic + map_set_rest_length;
 		op = reg[rrr];

@@ -1501,7 +1535,7 @@
 		do {
 		  for (;map_set_rest_length > 0;i++, ic++, map_set_rest_length--)
 		    {
-		      point = XINT(ccl_prog[ic]);
+		      GET_CCL_INT (point, ccl_prog, ic);
 		      if (point < 0)
 			{
 			  /* +1 is for including separator. */
@@ -1531,18 +1565,19 @@
 		      /* check map type,
 			 [STARTPOINT VAL1 VAL2 ...] or
 			 [t ELEMENT STARTPOINT ENDPOINT]  */
-		      if (NUMBERP (content))
+		      if (INTEGERP (content))
 			{
-			  point = XUINT (content);
-			  point = op - point + 1;
-			  if (!((point >= 1) && (point < size))) continue;
-			  content = AREF (map, point);
+			  point = XINT (content);
+			  if (!(point <= op && op - point + 1 < size)) continue;
+			  content = AREF (map, op - point + 1);
 			}
 		      else if (EQ (content, Qt))
 			{
 			  if (size != 4) continue;
-			  if ((op >= XUINT (AREF (map, 2))) &&
-			      (op < XUINT (AREF (map, 3))))
+			  if (INTEGERP (AREF (map, 2))
+			      && XINT (AREF (map, 2)) <= op
+			      && INTEGERP (AREF (map, 3))
+			      && op < XINT (AREF (map, 3)))
 			    content = AREF (map, 1);
 			  else
 			    continue;
@@ -1554,7 +1589,7 @@
 			continue;

 		      reg[RRR] = i;
-		      if (NUMBERP (content))
+		      if (INTEGERP (content) && IN_INT_RANGE (XINT (content)))
 			{
 			  op = XINT (content);
 			  i += map_set_rest_length - 1;
@@ -1566,9 +1601,10 @@
 			{
 			  attrib = XCAR (content);
 			  value = XCDR (content);
-			  if (!NUMBERP (attrib) || !NUMBERP (value))
+			  if (! (INTEGERP (attrib) && INTEGERP (value)
+				 && IN_INT_RANGE (XINT (value))))
 			    continue;
-			  op = XUINT (value);
+			  op = XINT (value);
 			  i += map_set_rest_length - 1;
 			  ic += map_set_rest_length - 1;
 			  POP_MAPPING_STACK (map_set_rest_length, reg[rrr]);
@@ -1613,7 +1649,7 @@
 	    case CCL_MapSingle:
 	      {
 		Lisp_Object map, attrib, value, content;
-		int size, point;
+		int point;
 		j = XINT (ccl_prog[ic++]); /* map_id */
 		op = reg[rrr];
 		if (j >= ASIZE (Vcode_conversion_map_vector))
@@ -1628,41 +1664,36 @@
 		    break;
 		  }
 		map = XCDR (map);
-		if (!VECTORP (map))
+		if (! (VECTORP (map)
+		       && INTEGERP (AREF (map, 0))
+		       && XINT (AREF (map, 0)) <= op
+		       && op - XINT (AREF (map, 0)) + 1 < ASIZE (map)))
 		  {
 		    reg[RRR] = -1;
 		    break;
 		  }
-		size = ASIZE (map);
-		point = XUINT (AREF (map, 0));
+		point = XINT (AREF (map, 0));
 		point = op - point + 1;
 		reg[RRR] = 0;
-		if ((size <= 1) ||
-		    (!((point >= 1) && (point < size))))
+		content = AREF (map, point);
+		if (NILP (content))
 		  reg[RRR] = -1;
-		else
+		else if (INTEGERP (content))
+		  reg[rrr] = XINT (content);
+		else if (EQ (content, Qt));
+		else if (CONSP (content))
 		  {
-		    reg[RRR] = 0;
-		    content = AREF (map, point);
-		    if (NILP (content))
-		      reg[RRR] = -1;
-		    else if (NUMBERP (content))
-		      reg[rrr] = XINT (content);
-		    else if (EQ (content, Qt));
-		    else if (CONSP (content))
-		      {
-			attrib = XCAR (content);
-			value = XCDR (content);
-			if (!NUMBERP (attrib) || !NUMBERP (value))
-			  continue;
-			reg[rrr] = XUINT(value);
-			break;
-		      }
-		    else if (SYMBOLP (content))
-		      CCL_CALL_FOR_MAP_INSTRUCTION (content, ic);
-		    else
-		      reg[RRR] = -1;
+		    attrib = XCAR (content);
+		    value = XCDR (content);
+		    if (!INTEGERP (attrib) || !INTEGERP (value))
+		      continue;
+		    reg[rrr] = XINT(value);
+		    break;
 		  }
+		else if (SYMBOLP (content))
+		  CCL_CALL_FOR_MAP_INSTRUCTION (content, ic);
+		else
+		  reg[RRR] = -1;
 	      }
 	      break;