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

[Thomas Dickey]

On Sat, May 08, 2021 at 10:35:54PM +0100, Sergei Trofimovich wrote:
> 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?

no - I pick up warnings by test-builds. Depending on the type of change, I
do test-builds on one or more of my machines, and compare the logs.
Test-builds exercise 5-10 configurations, etc.  I've a few dozen machines,
and limited time...

        https://invisible-island.net/personal/lint-tools.html#proc_builds
 
> 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'

well that tells it to either ignore (or disable runtime checks) for a lot
of things:

https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html

The checks are useful for development, less useful in release code.

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

that hard-codes an assumption that it's using two's complement :-)
 
> --- 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

> --- 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;
>  }
>  


-- 
Thomas E. Dickey <dickey@invisible-island.net>
https://invisible-island.net
ftp://ftp.invisible-island.net

signature.asc
Description: PGP signature