[PATCH 0/1] prompt: Fix an off-by-one in the overf check in countpromt.

[PATCH 0/1] prompt: Fix an off-by-one in the overf check in countpromt.

[warepire.ml]

From: Warepire <Warepire@users.noreply.github.com>

Additionally to the commit message: I have only managed to test this
for my own use-cases, which is a very narrow subset of tests.

There is a similar check at the bottom of the method that may need
the same change. Please have this in consideration during the review.

Warepire (1):
  prompt: Fix an off-by-one in the overf check in countpromt.

 Src/prompt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.15.1

[PATCH 1/1] prompt: Fix an off-by-one in the overf check in countpromt.

[warepire.ml]

From: Warepire <Warepire@users.noreply.github.com>

This triggers overf when the prompt is exactly as wide as the
term, causes countprompt to count a 2-line prompt as 3 lines.
Which transmits an errorneus TCUP to the PTY. In some terminals
this causes the last line of the previous command to be erased.

Hexdumping the PTY in benign and trouble cases, for my term
TCUP is represented by "\x1b[A". The NOK dumps are when the
prompt is exactly as wide as the term.

OK chunk dump:
  0000  0d 1b 5b 30 6d 1b 5b 32 37 6d 1b 5b 32 34 6d 1b  ..[0m.[27m.[24m.
  0010  5b 4a 1b 5b 33 33 6d 7e 2f 76 74 65 2d 6e 67 2d  [J.[33m~/vte-ng-
  0020  30 2e 35 30 2e 32 2e 61 1b 5b 30 30 6d 20 0d 0a  0.50.2.a.[00m ..
  0030  1b 5b 33 37 6d 77 61 72 65 70 69 72 65 20 25 1b  .[37mwarepire %.
  0040  5b 30 30 6d 20 1b 5b 4b 1b 5b 36 34 43 1b 5b 30  [00m .[K.[64C.[0
  0050  31 3b 33 32 6d e2 9c 94 1b 5b 30 30 6d 20 49 4e  1;32m....[00m IN
  0060  53 1b 5b 36 39 44 0d 0d 1b 5b 41 1b 5b 3f 32 30  S.[69D...[A.[?20  <-- Correct number of TCUP
  0070  30 34 68 1b 5b 30 6d 1b 5b 32 37 6d 1b 5b 32 34  04h.[0m.[27m.[24
  0080  6d 1b 5b 4a 1b 5b 33 33 6d 7e 2f 76 74 65 2d 6e  m.[J.[33m~/vte-n
  0090  67 2d 30 2e 35 30 2e 32 2e 61 1b 5b 30 30 6d 20  g-0.50.2.a.[00m
  00a0  0d 0a 1b 5b 33 37 6d 77 61 72 65 70 69 72 65 20  ...[37mwarepire
  00b0  25 1b 5b 30 30 6d 20 1b 5b 4b 1b 5b 36 34 43 1b  %.[00m .[K.[64C.
  00c0  5b 30 31 3b 33 32 6d e2 9c 94 1b 5b 30 30 6d 20  [01;32m....[00m
  00d0  49 4e 53 1b 5b 36 39 44                          INS.[69D

NOK chunk dump:
  0000  0d 1b 5b 30 6d 1b 5b 32 37 6d 1b 5b 32 34 6d 1b  ..[0m.[27m.[24m.
  0010  5b 4a 1b 5b 33 33 6d 7e 2f 74 65 73 74 2d 64 69  [J.[33m~/test-di
  0020  72 2f 61 61 61 61 61 61 61 61 61 61 61 61 61 61  r/aaaaaaaaaaaaaa
  0030  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  0040  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  0050  61 61 61 61 61 2f 62 62 62 62 62 62 62 62 62 62  aaaaa/bbbbbbbbbb
  0060  62 62 62 2f 63 63 63 1b 5b 30 30 6d 20 0d 0a 1b  bbb/ccc.[00m ...
  0070  5b 33 37 6d 77 61 72 65 70 69 72 65 20 25 1b 5b  [37mwarepire %.[
  0080  30 30 6d 20 1b 5b 4b 1b 5b 36 34 43 1b 5b 30 31  00m .[K.[64C.[01
  0090  3b 33 32 6d e2 9c 94 1b 5b 30 30 6d 20 49 4e 53  ;32m....[00m INS
  00a0  1b 5b 36 39 44 0d 0d 1b 5b 41 1b 5b 41 1b 5b 3f  .[69D...[A.[A.[?  <--- Incorrect number of TCUP
  00b0  32 30 30 34 68 1b 5b 30 6d 1b 5b 32 37 6d 1b 5b  2004h.[0m.[27m.[
  00c0  32 34 6d 1b 5b 4a 1b 5b 33 33 6d 7e 2f 74 65 73  24m.[J.[33m~/tes
  00d0  74 2d 64 69 72 2f 61 61 61 61 61 61 61 61 61 61  t-dir/aaaaaaaaaa
  00e0  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  00f0  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  0100  61 61 61 61 61 61 61 61 61 2f 62 62 62 62 62 62  aaaaaaaaa/bbbbbb
  0110  62 62 62 62 62 62 62 2f 63 63 63 1b 5b 30 30 6d  bbbbbbb/ccc.[00m
  0120  20 0d 0a 1b 5b 33 37 6d 77 61 72 65 70 69 72 65   ...[37mwarepire
  0130  20 25 1b 5b 30 30 6d 20 1b 5b 4b 1b 5b 36 34 43   %.[00m .[K.[64C
  0140  1b 5b 30 31 3b 33 32 6d e2 9c 94 1b 5b 30 30 6d  .[01;32m....[00m
  0150  20 49 4e 53 1b 5b 36 39 44                        INS.[69D

NOK chunks from I/O channel:
0 chars and 0 bytes in 1 chunks left to process.
read/bp:
  0000  00 0d 1b 5b 30 6d 1b 5b 32 37 6d 1b 5b 32 34 6d  ...[0m.[27m.[24m
  0010  1b 5b 4a 1b 5b 33 33 6d 7e 2f 74 65 73 74 2d 64  .[J.[33m~/test-d
  0020  69 72 2f 61 61 61 61 61 61 61 61 61 61 61 61 61  ir/aaaaaaaaaaaaa
  0030  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  0040  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  0050  61 61 61 61 61 61 2f 62 62 62 62 62 62 62 62 62  aaaaaa/bbbbbbbbb
  0060  62 62 62 62 2f 63 63 63 1b 5b 30 30 6d 20 0d 0a  bbbb/ccc.[00m ..
  0070  1b 5b 33 37 6d 77 61 72 65 70 69 72 65 20 25 1b  .[37mwarepire %.
  0080  5b 30 30 6d 20                                   [00m
read 132/5355 bytes, again? yes, active? yes
read/bp:
  0000  00 1b 5b 4b 1b 5b 36 34 43 1b 5b 30 31 3b 33 32  ..[K.[64C.[01;32
  0010  6d e2 9c 94 1b 5b 30 30 6d 20 49 4e 53 1b 5b 36  m....[00m INS.[6
  0020  39 44                                            9D
read 165/5355 bytes, again? yes, active? yes
read/bp:
  0000  00 0d 0d 1b 5b 41 1b 5b 41 1b 5b 3f 32 30 30 34  ....[A.[A.[?2004  <--- Incorrect number of TCUP
  0010  68 1b 5b 30 6d 1b 5b 32 37 6d 1b 5b 32 34 6d 1b  h.[0m.[27m.[24m.
  0020  5b 4a 1b 5b 33 33 6d 7e 2f 74 65 73 74 2d 64 69  [J.[33m~/test-di
  0030  72 2f 61 61 61 61 61 61 61 61 61 61 61 61 61 61  r/aaaaaaaaaaaaaa
  0040  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  0050  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
  0060  61 61 61 61 61 2f 62 62 62 62 62 62 62 62 62 62  aaaaa/bbbbbbbbbb
  0070  62 62 62 2f 63 63 63 1b 5b 30 30 6d 20 0d 0a 1b  bbb/ccc.[00m ...
  0080  5b 33 37 6d 77 61 72 65 70 69 72 65 20 25 1b 5b  [37mwarepire %.[
  0090  30 30 6d 20                                      00m
read 312/5355 bytes, again? yes, active? yes
read/bp:
  0000  00 1b 5b 4b 1b 5b 36 34 43 1b 5b 30 31 3b 33 32  ..[K.[64C.[01;32
  0010  6d e2 9c 94 1b 5b 30 30 6d 20 49 4e 53 1b 5b 36  m....[00m INS.[6
  0020  39 44                                            9D

Fixes: commit fa699be45debba815ea5e3ed5bfebde280d894a0
Signed-off-by: Warepire <Warepire@users.noreply.github.com>
---
 Src/prompt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Src/prompt.c b/Src/prompt.c
index c478e69fb..9b9e33670 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -1087,7 +1087,7 @@ countprompt(char *str, int *wp, int *hp, int overf)
 #endif
 
     for (; *str; str++) {
-	if (w >= zterm_columns && overf >= 0) {
+	if (w > zterm_columns && overf >= 0) {
 	    w = 0;
 	    h++;
 	}
-- 
2.15.1

Re: [PATCH 0/1] prompt: Fix an off-by-one in the overf check in countpromt.

[dana]

On 15 Jan 2018, at 04:35, warepire.ml@gmail.com wrote:
>Additionally to the commit message: I have only managed to test this
>for my own use-cases, which is a very narrow subset of tests.

FYI, if it's any help, i tested this in the following, too:

  * Terminal.app
  * iTerm2
  * Alacritty

I used a multi-line prompt where the top line had a length of COLUMNS, COLUMNS +
1, or COLUMNS - 1. I tried both with single-byte characters only and with mixed
single/multi-byte characters.

Replication:

  % zle-line-init() zle reset-prompt
  % zle -N zle-line-init
  % PROMPT=${(l<$COLUMNS><abc>):-}$'\n%~ %# '
  abcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabcabc
  ~ % echo foo

Current behaviour: The echo output is swallowed up by the next prompt.
Patched behaviour: The echo output remains unmolested.

PS: When i was testing it i had changed the comparison in both places; as
mentioned, the patch as submitted only changes the first one.

dana

Re: [PATCH 1/1] prompt: Fix an off-by-one in the overf check in countpromt.

[Bart Schaefer]

On Mon, Jan 15, 2018 at 2:35 AM,  <warepire.ml@gmail.com> wrote:
> From: Warepire <Warepire@users.noreply.github.com>
>
> This triggers overf when the prompt is exactly as wide as the
> term, causes countprompt to count a 2-line prompt as 3 lines.
> Which transmits an errorneus TCUP to the PTY. In some terminals
> this causes the last line of the previous command to be erased.

I see this has already been pushed to the zsh git, but I'm not sure
it's entirely correct.

This may cause problems on some terminals where auto-margin causes an
additional linefeed when the rightmost (or in some cases only the
bottom rightmost) character position is written.  I would be willing
to bet that the original code was intended to account for this, and
other code has changed around it.

The correct thing may be to make this conditional upon
ZLE_RPROMPT_INDENT, or introduce a similar setting to override the
terminfo assertions of auto-wrap/auto-margin so that the user can
assert whether the additional TCUP is needed.

Re: [PATCH 1/1] prompt: Fix an off-by-one in the overf check in countpromt.

[Warepire -]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

This is what I was worried about, hence why I included the cover letter.

I just tried setting ZLE_RPROMPT_INDENT=0 in my .zshrc and my terminal
freaked out, it had no idea what to do with the cursor after I reached the
bottom corner. Length or wraps of my LPROMPT didn't matter much since I
have a multi-line config. The LPROMPT will not touch that character
position, but the RPROMPT does. However, this would most likely indeed
freak out on single-line LPROMPTS, although I could not come up with a
config that'd do it.

I don't think doing this based on ZLE_RPROMT_INDENT is the way to go,
libvte based terminals require the number of TCUPs to match the number of
lines the final prompt will have when drawn. I am open for fixing this
patch, if we can reach an agreement on what the proper handling should be.
I am sadly not clever enough about shells to come up with something on my
own here. I am available in the IRC channel for discussion if that would
help the decision making, since I can test configs quite quickly that way.


On Tue, Jan 16, 2018 at 4:28 PM, Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Mon, Jan 15, 2018 at 2:35 AM,  <warepire.ml@gmail.com> wrote:
> > From: Warepire <Warepire@users.noreply.github.com>
> >
> > This triggers overf when the prompt is exactly as wide as the
> > term, causes countprompt to count a 2-line prompt as 3 lines.
> > Which transmits an errorneus TCUP to the PTY. In some terminals
> > this causes the last line of the previous command to be erased.
>
> I see this has already been pushed to the zsh git, but I'm not sure
> it's entirely correct.
>
> This may cause problems on some terminals where auto-margin causes an
> additional linefeed when the rightmost (or in some cases only the
> bottom rightmost) character position is written.  I would be willing
> to bet that the original code was intended to account for this, and
> other code has changed around it.
>
> The correct thing may be to make this conditional upon
> ZLE_RPROMPT_INDENT, or introduce a similar setting to override the
> terminfo assertions of auto-wrap/auto-margin so that the user can
> assert whether the additional TCUP is needed.
>

Re: [PATCH 1/1] prompt: Fix an off-by-one in the overf check in countpromt.

[Bart Schaefer]

On Tue, Jan 16, 2018 at 9:04 AM, Warepire - <warepire.ml@gmail.com> wrote:
>
> I don't think doing this based on ZLE_RPROMT_INDENT is the way to go, libvte
> based terminals require the number of TCUPs to match the number of lines the
> final prompt will have when drawn. I am open for fixing this patch, if we
> can reach an agreement on what the proper handling should be.

The situation is and pretty much always has been intolerable.  Some
terminals linefeed as soon as the rightmost column is filled, and most
but not all of those swallow a newline if it is the next character
sent.  Some feed when anything, even a zero-width character or control
sequence, is sent when there is something in the rightmost column;
others do so only when the next character sent is printable.  Others
behave in one of these ways only for the bottomost-rightmost position.
Some have a "bug compatibility mode" which means you don't know
whether they're going to do this lunacy or not.  And the available
terminfo bits don't adequately describe all the combinations,
especially in the "bug compatibility" case.

One of the reasons zsh introduced the "prompt truncation" escapes is
so that the user can control whether his prompt ever reaches the
rightmost column, because there's no way for the C code to get this
right in all cases.

So whether we keep this change or one like it depends in part on which
behavior we think is now most common, because somebody is going to
have to do a workaround in their prompt settings no matter what we do.

Re: [PATCH 1/1] prompt: Fix an off-by-one in the overf check in countpromt.

[Warepire -]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

On Tue, Jan 16, 2018 at 8:32 PM, Bart Schaefer <schaefer@brasslantern.com>
wrote:

> On Tue, Jan 16, 2018 at 9:04 AM, Warepire - <warepire.ml@gmail.com> wrote:
> >
> > I don't think doing this based on ZLE_RPROMT_INDENT is the way to go,
> libvte
> > based terminals require the number of TCUPs to match the number of lines
> the
> > final prompt will have when drawn. I am open for fixing this patch, if we
> > can reach an agreement on what the proper handling should be.
>
> The situation is and pretty much always has been intolerable.  Some
> terminals linefeed as soon as the rightmost column is filled, and most
> but not all of those swallow a newline if it is the next character
> sent.  Some feed when anything, even a zero-width character or control
> sequence, is sent when there is something in the rightmost column;
> others do so only when the next character sent is printable.  Others
> behave in one of these ways only for the bottomost-rightmost position.
> Some have a "bug compatibility mode" which means you don't know
> whether they're going to do this lunacy or not.  And the available
> terminfo bits don't adequately describe all the combinations,
> especially in the "bug compatibility" case.
>
> One of the reasons zsh introduced the "prompt truncation" escapes is
> so that the user can control whether his prompt ever reaches the
> rightmost column, because there's no way for the C code to get this
> right in all cases.
>
> So whether we keep this change or one like it depends in part on which
> behavior we think is now most common, because somebody is going to
> have to do a workaround in their prompt settings no matter what we do.
>

I might have worded myself badly, if so, I apologize. What I meant with
proper handling was how to properly introduce a configuration setting for
this and determine it's default numerical value (or boolean?), since my
patch in it's current state may trigger the problems zsh wants to avoid (at
least in theory).
Checking through the terminfo and termcap manuals, with my poor
understanding there doesn't seem to be a (combination of) termcap(s) to
detect this safely enough to attempt it.

I have no idea what it should be called, but I am leaning on a default as
1, turning the check into "> zterm_columns - offset", and just like
ZLE_RPROMPT_INDENT users can set it to 0 if their terminal supports it (and
fixes the erased-output problem without freak-out). As the freak-outs are
much worse than the dropped line of output when comparing the possible
problems users may see by default.