bug#42660: emacs-27.1-rc1 and UBsan findings

bug#42660: emacs-27.1-rc1 and UBsan findings

[Jeffrey Walton]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

I believe these additional failures are due to -fsanitize=undefined
-fno-sanitize-recover=all.

SUMMARY OF TEST RESULTS
-----------------------
Files examined: 267
Ran 3850 tests, 7 failed to run, 3763 results as expected, 1
unexpected, 86 skipped
1 files did not contain any tests:
  src/emacs-module-tests.log
1 files did not finish:
  lisp/international/ccl-tests.log
1 files contained unexpected results:
  lisp/cedet/srecode-utest-template.log
Makefile:319: recipe for target 'check-doit' failed
make[2]: *** [check-doit] Error 2
make[2]: Leaving directory '/home/jwalton/Build-Scripts/emacs-27.1/test'
Makefile:289: recipe for target 'check' failed
make[1]: *** [check] Error 2
make[1]: Leaving directory '/home/jwalton/Build-Scripts/emacs-27.1/test'
Makefile:959: recipe for target 'check' failed
make: *** [check] Error 2

Here's a typical failure:

$ cat emacs-27.1/test/lisp/international/ccl-tests.log
Running 7 tests (2020-08-01 16:28:12-0400, selector `(not (or (tag
:expensive-test) (tag :unstable)))')
   passed  1/7  ccl-compile-midi (0.000181 sec)
   passed  2/7  ccl-compile-pgg (0.000093 sec)
   passed  3/7  ccl-dump-midi (0.005956 sec)
   passed  4/7  ccl-dump-pgg (0.000111 sec)
Package pgg-def is deprecated
Package pgg-parse is deprecated
Package pgg is deprecated
ccl.c:1146:29: runtime error: left shift of 1246883553 by 1 places
cannot be represented in type 'int'

[-- Attachment #2: config.log.zip --]
[-- Type: application/zip, Size: 44797 bytes --]

[-- Attachment #3: test-suite.log.zip --]
[-- Type: application/zip, Size: 146397 bytes --]

bug#42660: emacs-27.1-rc1 and UBsan findings

[Lars Ingebrigtsen]

Jeffrey Walton <noloader@gmail.com> writes:

> $ cat emacs-27.1/test/lisp/international/ccl-tests.log
> Running 7 tests (2020-08-01 16:28:12-0400, selector `(not (or (tag
> :expensive-test) (tag :unstable)))')
>    passed  1/7  ccl-compile-midi (0.000181 sec)
>    passed  2/7  ccl-compile-pgg (0.000093 sec)
>    passed  3/7  ccl-dump-midi (0.005956 sec)
>    passed  4/7  ccl-dump-pgg (0.000111 sec)
> Package pgg-def is deprecated
> Package pgg-parse is deprecated
> Package pgg is deprecated
> ccl.c:1146:29: runtime error: left shift of 1246883553 by 1 places
> cannot be represented in type 'int'

I get the same, basically:

Running 7 tests (2020-08-10 00:55:35+0200, selector `(not (tag :unstable))')
   passed  1/7  ccl-compile-midi (0.000614 sec)
   passed  2/7  ccl-compile-pgg (0.000371 sec)
   passed  3/7  ccl-dump-midi (0.001032 sec)
   passed  4/7  ccl-dump-pgg (0.000531 sec)
Package pgg-def is deprecated
Package pgg-parse is deprecated
Package pgg is deprecated
ccl.c:1153:29: runtime error: left shift of 1239426054 by 1 places cannot be represented in type 'int'
make[1]: *** [Makefile:183: lisp/international/ccl-tests.log] Error 1

This is the test that fails:

(ert-deftest pgg-parse-crc24 ()
  ;; Compiler
  (require 'pgg)
  (should (equal pgg-parse-crc24 prog-pgg-code))
  ;; Interpreter
  (should (equal (pgg-parse-crc24-string "foo") (concat [#x4f #xc2 #x55])))
  (should (equal (pgg-parse-crc24-string "bar") (concat [#x51 #xd9 #x53])))
  (should (equal (pgg-parse-crc24-string "baz") (concat [#xf0 #x58 #x6a]))))

All three of those pgg-parse calls make Emacs signal a runtime error
with this checking turned on.

So this is with:

CFLAGS="-fsanitize=undefined -fno-sanitize-recover=all" ./configure && make

Now, pgg is obsolete, so that's a test that probably should go away.
So is this a bug?  The thing that fails is this:

	    case CCL_LSH: reg[rrr] <<= i; break;

So it's doing a left shift on a too-high number...  which is something
that pgg does, but since that's obsolete, do we care?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#42660: emacs-27.1-rc1 and UBsan findings

[Lars Ingebrigtsen]

(Please keep the debbugs address in the Cc field.)

Jeffrey Walton <noloader@gmail.com> writes:

>> So it's doing a left shift on a too-high number...  which is something
>> that pgg does, but since that's obsolete, do we care?
>
> Integer wrap is well defined. Cast reg[rrr] to an unsigned. If
> reg[rrr] is left as signed, then it overflows (sign change) and
> wanders into undefined behavior and is subject to removal.
>
> Or you can remove the test.

It would obviously be better to fix the undefined behaviour (if the
compiler is correct here?), but as the only thing that seems to tickle
this behaviour is an obsolete package, I'm not sure we care?

But perhaps we do?

The CCL isn't the most popular stuff in Emacs these days...

bug#42660: emacs-27.1-rc1 and UBsan findings

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 179 bytes --]

The bug is unlikely to lead to a real problem, so I installed the attached 
low-priority patch into the master branch rather than into emacs-27. Closing the 
bug report as fixed.

[-- Attachment #2: 0001-Fix-glitch-uncovered-by-gcc-fsanitize-undefined.patch --]
[-- Type: text/x-patch, Size: 4910 bytes --]

From 9905001e4b0c9dc0a90cefdd9530a90d07a17b99 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 17 Aug 2020 17:54:44 -0700
Subject: [PATCH] Fix glitch uncovered by gcc -fsanitize=undefined

* src/ccl.c (ccl_driver): Defend against signed integer
overflow (Bug#42660).  Perhaps some of this is unnecessary,
but it is safe and ccl.c is low-priority these days.
---
 src/ccl.c | 104 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 85 insertions(+), 19 deletions(-)

diff --git a/src/ccl.c b/src/ccl.c
index e85cfa6cdf..86debeef0e 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -1142,19 +1142,52 @@ #define EXCMD (field1 >> 6)
 	ccl_expr_self:
 	  switch (op)
 	    {
-	    case CCL_PLUS: reg[rrr] += i; break;
-	    case CCL_MINUS: reg[rrr] -= i; break;
-	    case CCL_MUL: reg[rrr] *= i; break;
-	    case CCL_DIV: reg[rrr] /= i; break;
+	    case CCL_PLUS: INT_ADD_WRAPV (reg[rrr], i, &reg[rrr]); break;
+	    case CCL_MINUS: INT_SUBTRACT_WRAPV (reg[rrr], i, &reg[rrr]); break;
+	    case CCL_MUL: INT_MULTIPLY_WRAPV (reg[rrr], i, &reg[rrr]); break;
+	    case CCL_DIV:
+	      if (!i)
+		CCL_INVALID_CMD;
+	      if (!INT_DIVIDE_OVERFLOW (reg[rrr], i))
+		reg[rrr] /= i;
+	      break;
 	    case CCL_MOD: reg[rrr] %= i; break;
+	      if (!i)
+		CCL_INVALID_CMD;
+	      reg[rrr] = i == -1 ? 0 : reg[rrr] % i;
+	      break;
 	    case CCL_AND: reg[rrr] &= i; break;
 	    case CCL_OR: reg[rrr] |= i; break;
 	    case CCL_XOR: reg[rrr] ^= i; break;
-	    case CCL_LSH: reg[rrr] <<= i; break;
-	    case CCL_RSH: reg[rrr] >>= i; break;
-	    case CCL_LSH8: reg[rrr] <<= 8; reg[rrr] |= i; break;
+	    case CCL_LSH:
+	      if (i < 0)
+		CCL_INVALID_CMD;
+	      reg[rrr] = i < UINT_WIDTH ? (unsigned) reg[rrr] << i : 0;
+	      break;
+	    case CCL_RSH:
+	      if (i < 0)
+		CCL_INVALID_CMD;
+	      reg[rrr] = reg[rrr] >> min (i, INT_WIDTH - 1);
+	      break;
+	    case CCL_LSH8:
+	      reg[rrr] = (unsigned) reg[rrr] << 8;
+	      reg[rrr] |= i;
+	      break;
 	    case CCL_RSH8: reg[7] = reg[rrr] & 0xFF; reg[rrr] >>= 8; break;
-	    case CCL_DIVMOD: reg[7] = reg[rrr] % i; reg[rrr] /= i; break;
+	    case CCL_DIVMOD:
+	      if (!i)
+		CCL_INVALID_CMD;
+	      if (i == -1)
+		{
+		  reg[7] = 0;
+		  INT_SUBTRACT_WRAPV (0, reg[rrr], &reg[rrr]);
+		}
+	      else
+		{
+		  reg[7] = reg[rrr] % i;
+		  reg[rrr] /= i;
+		}
+	      break;
 	    case CCL_LS: reg[rrr] = reg[rrr] < i; break;
 	    case CCL_GT: reg[rrr] = reg[rrr] > i; break;
 	    case CCL_EQ: reg[rrr] = reg[rrr] == i; break;
@@ -1204,19 +1237,52 @@ #define EXCMD (field1 >> 6)
 	ccl_set_expr:
 	  switch (op)
 	    {
-	    case CCL_PLUS: reg[rrr] = i + j; break;
-	    case CCL_MINUS: reg[rrr] = i - j; break;
-	    case CCL_MUL: reg[rrr] = i * j; break;
-	    case CCL_DIV: reg[rrr] = i / j; break;
-	    case CCL_MOD: reg[rrr] = i % j; break;
+	    case CCL_PLUS: INT_ADD_WRAPV (i, j, &reg[rrr]); break;
+	    case CCL_MINUS: INT_SUBTRACT_WRAPV (i, j, &reg[rrr]); break;
+	    case CCL_MUL: INT_MULTIPLY_WRAPV (i, j, &reg[rrr]); break;
+	    case CCL_DIV:
+	      if (!j)
+		CCL_INVALID_CMD;
+	      if (!INT_DIVIDE_OVERFLOW (i, j))
+		i /= j;
+	      reg[rrr] = i;
+	      break;
+	    case CCL_MOD:
+	      if (!j)
+		CCL_INVALID_CMD;
+	      reg[rrr] = j == -1 ? 0 : i % j;
+	      break;
 	    case CCL_AND: reg[rrr] = i & j; break;
 	    case CCL_OR: reg[rrr] = i | j; break;
 	    case CCL_XOR: reg[rrr] = i ^ j; break;
-	    case CCL_LSH: reg[rrr] = i << j; break;
-	    case CCL_RSH: reg[rrr] = i >> j; break;
-	    case CCL_LSH8: reg[rrr] = (i << 8) | j; break;
+	    case CCL_LSH:
+	      if (j < 0)
+		CCL_INVALID_CMD;
+	      reg[rrr] = j < UINT_WIDTH ? (unsigned) i << j : 0;
+	      break;
+	    case CCL_RSH:
+	      if (j < 0)
+		CCL_INVALID_CMD;
+	      reg[rrr] = i >> min (j, INT_WIDTH - 1);
+	      break;
+	    case CCL_LSH8:
+	      reg[rrr] = ((unsigned) i << 8) | j;
+	      break;
 	    case CCL_RSH8: reg[rrr] = i >> 8; reg[7] = i & 0xFF; break;
-	    case CCL_DIVMOD: reg[rrr] = i / j; reg[7] = i % j; break;
+	    case CCL_DIVMOD:
+	      if (!j)
+		CCL_INVALID_CMD;
+	      if (j == -1)
+		{
+		  INT_SUBTRACT_WRAPV (0, reg[rrr], &reg[rrr]);
+		  reg[7] = 0;
+		}
+	      else
+		{
+		  reg[rrr] = i / j;
+		  reg[7] = i % j;
+		}
+	      break;
 	    case CCL_LS: reg[rrr] = i < j; break;
 	    case CCL_GT: reg[rrr] = i > j; break;
 	    case CCL_EQ: reg[rrr] = i == j; break;
@@ -1225,7 +1291,7 @@ #define EXCMD (field1 >> 6)
 	    case CCL_NE: reg[rrr] = i != j; break;
 	    case CCL_DECODE_SJIS:
 	      {
-		i = (i << 8) | j;
+		i = ((unsigned) i << 8) | j;
 		SJIS_TO_JIS (i);
 		reg[rrr] = i >> 8;
 		reg[7] = i & 0xFF;
@@ -1233,7 +1299,7 @@ #define EXCMD (field1 >> 6)
 	      }
 	    case CCL_ENCODE_SJIS:
 	      {
-		i = (i << 8) | j;
+		i = ((unsigned) i << 8) | j;
 		JIS_TO_SJIS (i);
 		reg[rrr] = i >> 8;
 		reg[7] = i & 0xFF;
-- 
2.17.1