NetBSD Problem Report #56224
From www@netbsd.org Tue Jun 1 00:18:52 2021
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
by mollari.NetBSD.org (Postfix) with ESMTPS id B7DA51A928F
for <gnats-bugs@gnats.NetBSD.org>; Tue, 1 Jun 2021 00:18:52 +0000 (UTC)
Message-Id: <20210601001851.602411A92C9@mollari.NetBSD.org>
Date: Tue, 1 Jun 2021 00:18:51 +0000 (UTC)
From: mforney@mforney.org
Reply-To: mforney@mforney.org
To: gnats-bugs@NetBSD.org
Subject: libcurses: adding character to last line of non-scrolling window wraps instead of truncates
X-Send-Pr-Version: www-1.0
>Number: 56224
>Category: lib
>Synopsis: libcurses: adding character to last line of non-scrolling window wraps instead of truncates
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: blymn
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Tue Jun 01 00:20:00 +0000 2021
>Closed-Date: Mon Apr 25 21:50:15 +0000 2022
>Last-Modified: Mon Apr 25 21:50:15 +0000 2022
>Originator: Michael Forney
>Release:
>Organization:
>Environment:
>Description:
X/Open curses states[0] that when adding a string of characters,
> If scrolling is disabled, any characters that would extend beyond
> the last column of the last line are truncated.
However, NetBSD's libcurses instead clears the line, and wraps back to the start of the last line.
[0] https://pubs.opengroup.org/onlinepubs/7908799/xcurses/intov.html#tag_001_004_002_002
>How-To-Repeat:
This problem was encounter when running catgirl (https://git.causal.agency/catgirl/about/) with NetBSD's libcurses. When the status window (used to show channels which have unread messages) exceeds the terminal width, it is incorrectly wrapped around, showing only the last few channels.
Below is a test program demonstrating the problem. When the terminal is resized below 72 columns, the string wraps around to the beginning of the line instead of getting truncated.
#include <curses.h>
int main(void) {
initscr();
do {
clear();
move(LINES - 1, 0);
addstr("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789!@#$%^&*()");
refresh();
} while (getch() != 'q');
endwin();
return 0;
}
>Fix:
>Release-Note:
>Audit-Trail:
Responsible-Changed-From-To: lib-bug-people->blymn
Responsible-Changed-By: blymn@NetBSD.org
Responsible-Changed-When: Sat, 05 Jun 2021 06:15:49 +0000
Responsible-Changed-Why:
I shall claim this one.
From: "Brett Lymn" <blymn@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/56224 CVS commit: src/tests/lib/libcurses
Date: Sun, 6 Jun 2021 04:57:58 +0000
Module Name: src
Committed By: blymn
Date: Sun Jun 6 04:57:58 UTC 2021
Modified Files:
src/tests/lib/libcurses/check_files: Makefile slk1.chk slk3.chk
slk5.chk slk6.chk slk_init.chk
src/tests/lib/libcurses/tests: addstr slk
Added Files:
src/tests/lib/libcurses/check_files: addstr2.chk addstr3.chk
Log Message:
New tests and updates for corrected behaviour due to fix for PR56224
* Added extra testing to the addstr test to demonstrate bug described
in PR#56224 and validate case when scrolling enabled still works.
* Fixed slk test, the slk_init output changed due to corrected wrapping,
slk_restore no longer returns ERR probably due to addwchar no longer
returning ERR when an implicit scroll was attempted when scrolling
disabled. Commented out the slk_wset test, this is now returning ERR
instead of misbehaving, needs investigation.
To generate a diff of this commit:
cvs rdiff -u -r1.6 -r1.7 src/tests/lib/libcurses/check_files/Makefile
cvs rdiff -u -r0 -r1.1 src/tests/lib/libcurses/check_files/addstr2.chk \
src/tests/lib/libcurses/check_files/addstr3.chk
cvs rdiff -u -r1.1 -r1.2 src/tests/lib/libcurses/check_files/slk1.chk \
src/tests/lib/libcurses/check_files/slk3.chk \
src/tests/lib/libcurses/check_files/slk5.chk \
src/tests/lib/libcurses/check_files/slk6.chk \
src/tests/lib/libcurses/check_files/slk_init.chk
cvs rdiff -u -r1.2 -r1.3 src/tests/lib/libcurses/tests/addstr
cvs rdiff -u -r1.1 -r1.2 src/tests/lib/libcurses/tests/slk
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->feedback
State-Changed-By: blymn@NetBSD.org
State-Changed-When: Sun, 06 Jun 2021 05:08:00 +0000
State-Changed-Why:
Fix committed, requires feedback from the submitter.
From: "Brett Lymn" <blymn@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/56224 CVS commit: src/lib/libcurses
Date: Sun, 6 Jun 2021 05:06:44 +0000
Module Name: src
Committed By: blymn
Date: Sun Jun 6 05:06:44 UTC 2021
Modified Files:
src/lib/libcurses: addbytes.c
Log Message:
Fix for PR lib/56224
Correct addstr behaviour so it truncates the string in the case where
a string is added on the bottom line of a window where scrolling is
disabled as per the SUSv2 specification.
To generate a diff of this commit:
cvs rdiff -u -r1.54 -r1.55 src/lib/libcurses/addbytes.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: lib/56224: libcurses: adding character to last line of
non-scrolling window wraps instead of truncates
Date: Sun, 6 Jun 2021 14:41:41 +0930
On Tue, Jun 01, 2021 at 12:20:00AM +0000, mforney@mforney.org wrote:
>
> Below is a test program demonstrating the problem. When the terminal is resized below 72 columns, the string wraps around to the beginning of the line instead of getting truncated.
>
I have just committed a fix for this, please update libcurses, rebuild
and reinstall it.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
From: Michael Forney <mforney@mforney.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: lib/56224: libcurses: adding character to last line of
non-scrolling window wraps instead of
Date: Wed, 09 Jun 2021 20:54:03 -0700
Brett Lymn <blymn@internode.on.net> wrote:
> I have just committed a fix for this, please update libcurses, rebuild
> and reinstall it.
Thanks for the patch, I can confirm that this does solve the issue
for the application in question (catgirl).
However, I did a bit of investigation myself, and have a few comments:
1. It seems that now the string is truncated by one extra character
(the last column is empty). Since this last character doesn't extend
beyond the end of the line, my expectation is that it should not
be truncated. And indeed, this is how it behaves in ncurses.
2. I was thinking that this needed to be fixed in _cursesi_addwchar,
not _cursesi_addwbytes. Otherwise, addwstr and addch at the last
column still wrap to column 0 (though, addwstr already truncates
since it returns on the first ERR, whereas _curses_waddbytes ignores
all errors except for the last character).
3. This seems incorrect to me:
n =3D (int)mbrtowc(&wc, bytes, (size_t)count, &st);
...
/* if scrollok is false and we are at the bottom of
* screen and this character would take us past the
* end of the line then we are done.
*/
if ((win->curx + n >=3D win->maxx) &&
(!(win->flags & __SCROLLOK)) &&
(win->cury =3D=3D win->scr_b))
break;
Here, n is the number of bytes in a multi-byte character, *not* the
column width of that character. For example, consider this modified
test program:
#include <curses.h>
#include <locale.h>
=09
int main(void) {
setlocale(LC_ALL, "C.UTF-8");
initscr();
noecho();
raw();
do {
clear();
mvaddstr(LINES - 1, COLS - 7, "foo=E2=86=92=E2=86=92=E2=86=92");
refresh();
} while (getch() !=3D 'q');
endwin();
return 0;
}
The string gets truncated after the first '=E2=86=92' (U+2192, 3 bytes in
UTF-8), even though there are 3 more columns available in the line.
If you replace '=E2=86=92' with '=C2=B5' (U+03BC, 2 bytes in UTF-8), it get=
s
truncated after the second one. If you replace it with '*', it does
not get truncated.
Below is an alternative fix I've been testing that seems to solve
these problems. The first thing it does is make _cursesi_waddbytes
break from the loop and return ERR when any of the _cursesi_addwchar
calls returns ERR (not just the last one). This is consistent with
waddnwstr. The second thing it does is delay wrapping the cursor x
position back to zero until after we return ERR if scrolling is not
allowed.
diff --git a/lib/libcurses/addbytes.c b/lib/libcurses/addbytes.c
index 0a85ae76c3ad..8533a60ed752 100644
--- a/lib/libcurses/addbytes.c
+++ b/lib/libcurses/addbytes.c
@@ -141,7 +141,7 @@ _cursesi_waddbytes(WINDOW *win, const char *bytes, int =
count, attr_t attr,
#ifdef HAVE_WCHAR
(void)memset(&st, 0, sizeof(st));
#endif
- while (count > 0) {
+ while (count > 0 && err =3D=3D OK) {
#ifndef HAVE_WCHAR
c =3D *bytes++;
#ifdef DEBUG
@@ -241,7 +241,6 @@ _cursesi_addbyte(WINDOW *win, __LINE **lp, int *y, int =
*x, int c,
#endif
=20
if (char_interp && ((*lp)->flags & __ISPASTEOL)) {
- *x =3D 0;
(*lp)->flags &=3D ~__ISPASTEOL;
if (*y =3D=3D win->scr_b) {
#ifdef DEBUG
@@ -255,6 +254,7 @@ _cursesi_addbyte(WINDOW *win, __LINE **lp, int *y, int =
*x, int c,
} else {
(*y)++;
}
+ *x =3D 0;
*lp =3D win->alines[*y];
if (c =3D=3D '\n')
return OK;
@@ -350,7 +350,6 @@ _cursesi_addwchar(WINDOW *win, __LINE **lnp, int *y, in=
t *x,
return OK;
case L'\n':
wclrtoeol(win);
- *x =3D 0;
(*lnp)->flags &=3D ~__ISPASTEOL;
if (*y =3D=3D win->scr_b) {
if (!(win->flags & __SCROLLOK))
@@ -359,6 +358,7 @@ _cursesi_addwchar(WINDOW *win, __LINE **lnp, int *y, in=
t *x,
} else {
(*y)++;
}
+ *x =3D 0;
return OK;
case L'\t':
cc.vals[0] =3D L' ';
@@ -404,7 +404,6 @@ _cursesi_addwchar(WINDOW *win, __LINE **lnp, int *y, in=
t *x,
}
/* check for new line first */
if (char_interp && ((*lnp)->flags & __ISPASTEOL)) {
- *x =3D 0;
(*lnp)->flags &=3D ~__ISPASTEOL;
if (*y =3D=3D win->scr_b) {
if (!(win->flags & __SCROLLOK))
@@ -413,6 +412,7 @@ _cursesi_addwchar(WINDOW *win, __LINE **lnp, int *y, in=
t *x,
} else {
(*y)++;
}
+ *x =3D 0;
(*lnp) =3D win->alines[*y];
lp =3D &win->alines[*y]->line[*x];
}
@@ -468,7 +468,6 @@ _cursesi_addwchar(WINDOW *win, __LINE **lnp, int *y, in=
t *x,
if (newx > *(*lnp)->lastchp)
*(*lnp)->lastchp =3D newx;
__touchline(win, *y, sx, (int) win->maxx - 1);
- sx =3D *x =3D 0;
if (*y =3D=3D win->scr_b) {
if (!(win->flags & __SCROLLOK))
return ERR;
@@ -476,6 +475,7 @@ _cursesi_addwchar(WINDOW *win, __LINE **lnp, int *y, in=
t *x,
} else {
(*y)++;
}
+ sx =3D *x =3D 0;
lp =3D &win->alines[*y]->line[0];
(*lnp) =3D win->alines[*y];
}
@@ -556,14 +556,16 @@ _cursesi_addwchar(WINDOW *win, __LINE **lnp, int *y, =
int *x,
if (newx > *(*lnp)->lastchp)
*(*lnp)->lastchp =3D newx;
__touchline(win, *y, sx, (int) win->maxx - 1);
- *x =3D sx =3D 0;
if (*y =3D=3D win->scr_b) {
- if (!(win->flags & __SCROLLOK))
+ if (!(win->flags & __SCROLLOK)) {
+ *x =3D win->maxx - 1;
return ERR;
+ }
scroll(win);
} else {
(*y)++;
}
+ *x =3D sx =3D 0;
lp =3D &win->alines[*y]->line[0];
(*lnp) =3D win->alines[*y];
} else {
From: Michael Forney <mforney@mforney.org>
To: Brett Lymn <blymn@internode.on.net>
Cc: gnats-bugs@netbsd.org, blymn@netbsd.org, gnats-admin@netbsd.org,
netbsd-bugs@netbsd.org
Subject: Re: lib/56224: libcurses: adding character to last line of
non-scrolling window wraps instead of
Date: Thu, 10 Jun 2021 00:56:15 -0700
Thanks for the follow-up!
On 2021-06-09, Brett Lymn <blymn@internode.on.net> wrote:
>> 2. I was thinking that this needed to be fixed in _cursesi_addwchar,
>> not _cursesi_addwbytes. Otherwise, addwstr and addch at the last
>> column still wrap to column 0 (though, addwstr already truncates
>> since it returns on the first ERR, whereas _curses_waddbytes ignores
>> all errors except for the last character).
>>
>
> No, _cursesi_addwchar is used by other routines that really should be
> inheriting the same behaviour, rather than fix each one individually I
> went for fixing it in the common routine so they all behaved the same.
> I think addwstr is the outlier in this instance. I will look at fixing
> that.
Hmm... either I'm misunderstanding you, or you me. I agree that the
fix should be in _cursesi_addwchar rather than fixing each one
individually. _cursesi_waddbytes ends up at _cursesi_addwchar, but not
everything ends up at _cursesi_waddbytes. Fixing the wrap in
_cursesi_addwchar should fix it for everything.
> I may have read the SUSv2 section on the scrolling behaviour incorrectly
> but my interpretation is that trying to add a string at the bottom of
> the screen that would cause a scroll when scrollok is false should
> result in a silent truncation not ERR which is why I did what I did.
> The effect of my change is a slk routine that used to strangely return
> ERR no longer does so... I would like to keep that behaviour :)
Yeah, it doesn't seem to provide very many details on when exactly ERR
should be returned.
The current behavior is that the addch functions return ERR when
adding at the bottom right corner, and I think it makes sense to
return ERR from addstr functions when they encounter ERR from addch.
This is what addwstr already does and is at least consistent with
ncurses.
I am not familiar with the slk routines, so I can't comment on the
interaction with those. Perhaps they could ignore ERR from addstr if
they don't care about it?
> The patch you have proposed does not look right, with your fix you will
> have possibly not reset x to 0 but ISPASTEOL will be unset so the unset
> of the ISPASTEOL flag needs to be moved too which does make some sense
> to have that after a possible error return. I will have a look at doing
> that.
Yeah, I was not quite sure how __ISPASTEOL fit into the picture. I
agree that it only makes sense to clear it when the cursor is actually
wrapped.
One version of a patch I was testing had _cursesi_addwchar set
__ISPASTEOL when it fails to move the cursor past the end of the line
(line 598 of addbytes.c in current trunk), since this is what happens
in _curses_addbyte (with DISABLE_WCHAR, line 331). However, this had a
bad interaction with mvaddch, since the flag isn't cleared during
wmove. I do not quite understand why __ISPASTEOL is a per-line flag
instead of a per-window flag, since it seems to be a property of the
cursor, not the line. But perhaps this is a discussion for another
day.
From: Brett Lymn <blymn@internode.on.net>
To: gnats-bugs@netbsd.org
Cc: blymn@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org,
mforney@mforney.org
Subject: Re: lib/56224: libcurses: adding character to last line of
non-scrolling window wraps instead of
Date: Thu, 10 Jun 2021 16:01:14 +0930
On Thu, Jun 10, 2021 at 03:55:02AM +0000, Michael Forney wrote:
>
> Thanks for the patch, I can confirm that this does solve the issue
> for the application in question (catgirl).
>
Excellent, that is a good start.
>
> 1. It seems that now the string is truncated by one extra character
> (the last column is empty). Since this last character doesn't extend
> beyond the end of the line, my expectation is that it should not
> be truncated. And indeed, this is how it behaves in ncurses.
>
OK, I will fix that - my checks must be off by one.
> 2. I was thinking that this needed to be fixed in _cursesi_addwchar,
> not _cursesi_addwbytes. Otherwise, addwstr and addch at the last
> column still wrap to column 0 (though, addwstr already truncates
> since it returns on the first ERR, whereas _curses_waddbytes ignores
> all errors except for the last character).
>
No, _cursesi_addwchar is used by other routines that really should be
inheriting the same behaviour, rather than fix each one individually I
went for fixing it in the common routine so they all behaved the same.
I think addwstr is the outlier in this instance. I will look at fixing
that.
> 3. This seems incorrect to me:
>
Yes, it is. My bad. I will rework that. That probable explains why
the wide char version of the slk test failed after my updates.
>
> Below is an alternative fix I've been testing that seems to solve
> these problems. The first thing it does is make _cursesi_waddbytes
> break from the loop and return ERR when any of the _cursesi_addwchar
> calls returns ERR (not just the last one). This is consistent with
> waddnwstr. The second thing it does is delay wrapping the cursor x
> position back to zero until after we return ERR if scrolling is not
> allowed.
>
I may have read the SUSv2 section on the scrolling behaviour incorrectly
but my interpretation is that trying to add a string at the bottom of
the screen that would cause a scroll when scrollok is false should
result in a silent truncation not ERR which is why I did what I did.
The effect of my change is a slk routine that used to strangely return
ERR no longer does so... I would like to keep that behaviour :)
The patch you have proposed does not look right, with your fix you will
have possibly not reset x to 0 but ISPASTEOL will be unset so the unset
of the ISPASTEOL flag needs to be moved too which does make some sense
to have that after a possible error return. I will have a look at doing
that.
--
Brett Lymn
--
Sent from my NetBSD device.
"We are were wolves",
"You mean werewolves?",
"No we were wolves, now we are something else entirely",
"Oh"
From: "Brett Lymn" <blymn@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/56224 CVS commit: src/lib/libcurses
Date: Tue, 15 Jun 2021 22:18:55 +0000
Module Name: src
Committed By: blymn
Date: Tue Jun 15 22:18:55 UTC 2021
Modified Files:
src/lib/libcurses: addbytes.c
Log Message:
Correct a previous fix for PR lib/56224.
Use wdwitch to determine the width of a wide character on the screen not
the number from mbrtowc which is the number of bytes in the character.
Thanks to Michael Forney for spotting this.
To generate a diff of this commit:
cvs rdiff -u -r1.55 -r1.56 src/lib/libcurses/addbytes.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: feedback->closed
State-Changed-By: blymn@NetBSD.org
State-Changed-When: Mon, 25 Apr 2022 21:50:15 +0000
State-Changed-Why:
Closing bug, I believe it is fixed.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.46 2020/01/03 16:35:01 leot Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2020
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home