Date: Tue, 18 Feb 1997 16:21:25 +0100 (CET) From: Arne Henrik Juul <arnej@imf.unit.no> To: FreeBSD-gnats-submit@freebsd.org Subject: bin/2762: Precedence mistake in libncurses Message-ID: <199702181521.QAA16402@frida.imf.unit.no> Resent-Message-ID: <199702181530.HAA10019@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 2762
>Category: bin
>Synopsis: Precedence mistake in libncurses
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: freebsd-bugs
>State: open
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Tue Feb 18 07:30:02 PST 1997
>Last-Modified:
>Originator: Arne Henrik Juul
>Organization:
Norwegian University of Technology and Science
>Release: FreeBSD 2.2-GAMMA i386
>Environment:
Applies both to 2.2-GAMMA and 3.0-current as of Feb, 1997.
diff generated from a 3.0-current checkout.
>Description:
Some code in libncurses looks wrong, like somebody misunderstood
the C precedence rules for bit operators and ==. (Since it
is commonly agreed that those precedence rules are wrong, this
is not very surprising :-)
There is also a pretty strange macro definition:
BLANK is #defined as ' '|A_NORMAL without any parantheses.
A_NORMAL is 0, so using BLANK in comparisons like this:
if (*ptr != BLANK)
expands to
if (*ptr != ' '| 0x00000000 )
which evaluates like
if ((*ptr != ' ')|0)
which doesn't really hurt, but makes for some pretty silly
warnings. Also, it makes the code dependant upon A_NORMAL
actually being 0, which hurts modularity and abstraction.
A bit worse is the mistakes with & and ==, since this changes
the semantics to something I'm pretty sure was not intended.
This code:
if (win->_line[y][x]&A_CHARTEXT == ' ')
was probably meant to mask out attributes, then compare to space.
It will expand to
if (win->_line[y][x]& 0x000000ff == ' ')
and evaluate to
if (win->_line[y][x]& 0)
so the body never gets executed.
This should all be reported back to whomever maintains
libncurses, to make sure we are making the correct fix
and that they will fix it in their code.
I don't know who those maintainers are; sorry.
>How-To-Repeat:
make libncurses, and watch out for the warnings of
"suggest parentheses around comparison in operand of &" and "|".
>Fix:
This fix looks right to me, but maybe the ncurses
writers had some strange reason for their seemingly
wrong (and inconsistent) definition of BLANK. It
doesn't actually change any behaviour in regard to
BLANK, so it's pretty safe anyway.
Index: src/lib/libncurses/lib_addch.c
===================================================================
RCS file: /usr/cvs/src/lib/libncurses/lib_addch.c,v
retrieving revision 1.6
diff -u -r1.6 lib_addch.c
--- lib_addch.c 1996/09/26 01:08:27 1.6
+++ lib_addch.c 1997/02/18 14:58:15
@@ -56,7 +56,7 @@
T(("win attr = %x", win->_attrs));
ch |= win->_attrs;
- if (win->_line[y][x]&A_CHARTEXT == ' ')
+ if ((win->_line[y][x]&A_CHARTEXT) == ' ')
ch |= win->_bkgd;
else
ch |= (win->_bkgd&A_ATTRIBUTES);
Index: src/lib/libncurses/lib_bkgd.c
===================================================================
RCS file: /usr/cvs/src/lib/libncurses/lib_bkgd.c,v
retrieving revision 1.7
diff -u -r1.7 lib_bkgd.c
--- lib_bkgd.c 1996/12/22 14:24:49 1.7
+++ lib_bkgd.c 1997/02/18 14:58:15
@@ -27,7 +27,7 @@
T(("wbkgd(%x, %x) called", win, ch));
for (y = 0; y <= win->_maxy; y++)
for (x = 0; x <= win->_maxx; x++)
- if (win->_line[y][x]&A_CHARTEXT == ' ')
+ if ((win->_line[y][x]&A_CHARTEXT) == ' ')
win->_line[y][x] |= ch;
else
win->_line[y][x] |= (ch&A_ATTRIBUTES);
Index: src/lib/libncurses/lib_clrbot.c
===================================================================
RCS file: /usr/cvs/src/lib/libncurses/lib_clrbot.c,v
retrieving revision 1.3
diff -u -r1.3 lib_clrbot.c
--- lib_clrbot.c 1996/09/26 01:08:36 1.3
+++ lib_clrbot.c 1997/02/18 14:56:51
@@ -12,7 +12,7 @@
#include "curses.priv.h"
-#define BLANK ' '|A_NORMAL
+#define BLANK (' '|A_NORMAL)
int wclrtobot(WINDOW *win)
{
Index: src/lib/libncurses/lib_clreol.c
===================================================================
RCS file: /usr/cvs/src/lib/libncurses/lib_clreol.c,v
retrieving revision 1.3
diff -u -r1.3 lib_clreol.c
--- lib_clreol.c 1996/09/26 01:08:39 1.3
+++ lib_clreol.c 1997/02/18 14:56:58
@@ -12,7 +12,7 @@
#include "curses.priv.h"
-#define BLANK ' '|A_NORMAL
+#define BLANK (' '|A_NORMAL)
int wclrtoeol(WINDOW *win)
{
Index: src/lib/libncurses/lib_doupdate.c
===================================================================
RCS file: /usr/cvs/src/lib/libncurses/lib_doupdate.c,v
retrieving revision 1.11
diff -u -r1.11 lib_doupdate.c
--- lib_doupdate.c 1996/09/26 01:08:51 1.11
+++ lib_doupdate.c 1997/02/18 14:57:05
@@ -195,7 +195,7 @@
**
*/
-#define BLANK ' '|A_NORMAL
+#define BLANK (' '|A_NORMAL)
static void ClrUpdate(WINDOW *scr)
{
Index: src/lib/libncurses/lib_erase.c
===================================================================
RCS file: /usr/cvs/src/lib/libncurses/lib_erase.c,v
retrieving revision 1.3
diff -u -r1.3 lib_erase.c
--- lib_erase.c 1995/05/30 05:46:14 1.3
+++ lib_erase.c 1997/02/18 14:57:51
@@ -13,7 +13,7 @@
#include "curses.priv.h"
#include "terminfo.h"
-#define BLANK ' '
+#define BLANK (' '|A_NORMAL)
int werase(WINDOW *win)
{
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199702181521.QAA16402>