aligrudi / neatvi

A small vi/ex editor for editing UTF-8 text
http://litcave.rudi.ir/
320 stars 27 forks source link

Fix -Warray-bounds warnings #51

Closed phikal closed 2 years ago

phikal commented 2 years ago

I have GCC 12 on my system, and compiling neatvi triggered these warnings

cc -c -Wall -O2 reg.c
reg.c: In function ‘reg_putraw’:
reg.c:21:18: warning: array subscript [-128, 255] is outside array bounds of ‘char *[256]’ [-Warray-bounds]
   21 |         free(bufs[tolower(c)]);
      |              ~~~~^~~~~~~~~~~~
reg.c:6:14: note: while referencing ‘bufs’
    6 | static char *bufs[256];
      |              ^~~~
reg.c:22:13: warning: array subscript [-128, 255] is outside array bounds of ‘char *[256]’ [-Warray-bounds]
   22 |         bufs[tolower(c)] = buf;
      |         ~~~~^~~~~~~~~~~~
reg.c:6:14: note: while referencing ‘bufs’
    6 | static char *bufs[256];
      |              ^~~~

The issue is that tolower does not necessarily return a value between 0 and 255, though this is to my understanding the implicit understanding in the code?

aligrudi commented 2 years ago

Philip Kaludercic @.***> wrote:

I have GCC 12 on my system, and compiling neatvi triggered these warnings

cc -c -Wall -O2 reg.c
reg.c: In function ‘reg putraw’:
reg.c:21:18: warning: array subscript [-128, 255] is outside array bounds of ‘char *[256]’ [-Warray-bounds]
   21 |         free(bufs[tolower(c)]);
      |              ~~~~^~~~~~~~~~~~
reg.c:6:14: note: while referencing ‘bufs’
    6 | static char *bufs[256];
      |              ^~~~
reg.c:22:13: warning: array subscript [-128, 255] is outside array bounds of ‘char *[256]’ [-Warray-bounds]
   22 |         bufs[tolower(c)] = buf;
      |         ~~~~^~~~~~~~~~~~
reg.c:6:14: note: while referencing ‘bufs’
    6 | static char *bufs[256];
      |              ^~~~

The issue is that tolower does not necessarily return a value between 0 and 255, though this is to my understanding the implicit understanding in the code? You can view, comment on, or merge this pull request online at:

Actually, the problem is that tolower() expects a non-negative value or EOF. Otherwise, it may even cause a segmentation fault (some implementations use an array). If the input of tolower() is non-negative, the output is also non-negative. So AFAICS the warning is wrong.

On the other hand, it is not safe to pass values of type signed char to tolower(). So most of the time you see tolower((unsigned char) ..). In neatvi, a cast is omitted, only when the value is known to be non-negative (unless I missed it). Checking reg_put() calls, there are a few such problems. I will fix them shortly.

Thanks, Ali

phikal commented 2 years ago

So I should close the pull request?

phikal commented 2 years ago

Oops, just closed it anyway.