Re: -fsanitize=undefined detects undefined behaviour in signed shift overflow

[Sergei Trofimovich]

On Sat, May 08, 2021 at 04:31:46PM -0400, Thomas Dickey wrote:
> On Sat, May 08, 2021 at 08:55:48PM +0100, Sergei Trofimovich wrote:
> > Hello ncurses maintainers!
> > 
> > In search for an unrelated bug I built a few local tools with
> > `-fsanitize=undefined` gcc option to catch an suspected undefined
> > behaviour.
> > 
> > Among other things ncurses was flagged in ncurses-based applications as:
> >   ncurses/tinfo/read_entry.c:111:19:
> >     runtime error: left shift of 255 by 24 places cannot be represented in 
> > type 'int'
> > 
> > which looks like a real (perhaps minor) problem:
> 
> I'm aware of the warning, but don't see a way to detect the overflow
> and not elicit a warning about undefined behavior.
> 
> A patch would help discussion.
> 
> (earlier this week I noticed a change by someone to eliminate undefined
> behavior which doesn't produce a compiler warning with/without the change,
> _and_ the change causes incorrect behavior -- something to keep in mind)

Aha, I was not able to lookup past discussions. Does ncurses have an
automatic regression testing suite I could throw my patches at?

I see two possible fixes:

1. Add CFLAGS=-fwrapv as part of ncurses' ./configure. I'm not very well
   versed in autoconf and it's forks to try it out easily.

   The following does not produce warning and should preserve reasonable
   overflow semantics:
     $ ./configure --enable-termcap CFLAGS='-O2 -g -fsanitize=undefined 
-fwrapv' LDFLAGS='-O2 -g -fsanitize=undefined -fwrapv'

2. Use unsigned arithmetics (also attached as a separate file).

--- a/ncurses/tinfo/read_entry.c
+++ b/ncurses/tinfo/read_entry.c
@@ -98,28 +98,28 @@ convert_32bits(char *buf, NCURSES_INT2 *Numbers, int count)
     }
     return size;
 }
 #else
 static size_t
 convert_32bits(char *buf, NCURSES_INT2 *Numbers, int count)
 {
     int i, j;
     unsigned char ch;
     for (i = 0; i < count; i++) {
-       int value = 0;
+       unsigned value = 0;
        for (j = 0; j < SIZEOF_32BITS; ++j) {
            ch = UChar(*buf++);
-           value |= (ch << (8 * j));
+           value |= ((unsigned)ch << (8 * j));
        }
-       if (value == -1)
+       if (value == ~0u)
            Numbers[i] = ABSENT_NUMERIC;
-       else if (value == -2)
+       else if (value == ~0u - 1)
            Numbers[i] = CANCELLED_NUMERIC;
        else if (value > MAX_OF_TYPE(NCURSES_INT2))
            Numbers[i] = MAX_OF_TYPE(NCURSES_INT2);
        else
            Numbers[i] = (short) value;
        TR(TRACE_DATABASE, ("get Numbers[%d]=%d", i, Numbers[i]));
     }
     return SIZEOF_SHORT;
 }

-- 

  Sergei

ncurses-unsigned-arith.patch
Description: Text Data