Skip to content

Commit dd1efb4

Browse files
committed
pty: fix char overflow causing test failures (re: 8aef678)
As of the referenced commit, the pty.sh regression tests were massively failing on some systems (at least Linux/s390 and NetBSD/arm64) with failure message showing evidence of corrupted values. The problem was caused by the type of the 'restore' Match_t member being changed from int to char, on the apparent assumption that only values from char variables were to be stored in it. But that is not the case; 'restore' is set to -1 to indicate that no character is currently saved and none should be restored. So the obvious fix would be to simply revert the type of 'restore' back to int. However, I'm now seeing another, long-standing problem in the code. The whole program works with char* pointers, not unsigned char*. So on systems where char is signed, bytes with the 8th bit set (like UTF-8 characters) will produce negative values, which gets stored in Match_t's 'restore' as negative, which means restoring doesn't happen. We could get around that by using unsigned char* instead of char* everywhere, but that would be a large diff with lots of onerous typecasts to silence compiler warnings[*] as unsigned char values are passed to functions that expect char parameters. So, instead: src/cmd/builtin/pty.c: - Rename Match_t's 'restore' to 'save', keep char type. Only use it for storing char values, which fixes the problem. - Add 'saving' member, a flag indicating if the value in 'save' should be restored. This replaces the -1 value of 'restore'. - Adapt the code to use these, which is straightforward. Thanks to @phidebian for the report. Resolves: #962 [*] Those warnings don't normally show up, but do when compiling with -Wsign-compare -Wshorten-64-to-32 -Wsign-conversion -Wimplicit-int-conversion, as we now do as part of @JohnoKing's project to make ksh ready to handle large amounts of data in a 64-bit address space.
1 parent 7d0bddc commit dd1efb4

1 file changed

Lines changed: 21 additions & 18 deletions

File tree

src/cmd/builtin/pty.c

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -511,7 +511,8 @@ typedef struct Master_s
511511
char* prompt; /* peek prompt */
512512
ptrdiff_t cursor; /* cursor in buf, 0 if fresh line */
513513
int line; /* prompt line number */
514-
char restore; /* previous line save char */
514+
char save; /* previous line's saved char */
515+
char saving; /* set if saved char is to be restored */
515516
} Master_t;
516517
#define BUFUNDERFLOW 128 /* bytes of buffer underflow to allow */
517518

@@ -537,13 +538,13 @@ masterline(Sfio_t* mp, Sfio_t* lp, char* prompt, int must, int timeout, Master_t
537538
again:
538539
if (prompt)
539540
{
540-
if (bp->cur < bp->end && bp->restore >= 0)
541-
*bp->cur = bp->restore;
541+
if (bp->cur < bp->end && bp->saving)
542+
*bp->cur = bp->save;
542543
if (strneq(bp->cur, promptbuf, promptlen))
543544
r = bp->cur;
544545
else
545546
r = 0;
546-
if (bp->cur < bp->end && bp->restore >= 0)
547+
if (bp->cur < bp->end && bp->saving)
547548
*bp->cur = 0;
548549
if (r)
549550
{
@@ -571,10 +572,11 @@ masterline(Sfio_t* mp, Sfio_t* lp, char* prompt, int must, int timeout, Master_t
571572
}
572573
else if (bp->nxt)
573574
{
574-
if (bp->restore >= 0)
575-
*bp->cur = bp->restore;
575+
if (bp->saving)
576+
*bp->cur = bp->save;
576577
r = bp->cur;
577-
bp->restore = *bp->nxt;
578+
bp->save = *bp->nxt;
579+
bp->saving = 1;
578580
*bp->nxt = 0;
579581
if (bp->nxt >= bp->end)
580582
{
@@ -600,10 +602,10 @@ masterline(Sfio_t* mp, Sfio_t* lp, char* prompt, int must, int timeout, Master_t
600602
}
601603
else if (bp->cur < bp->end)
602604
{
603-
if (bp->restore >= 0)
605+
if (bp->saving)
604606
{
605-
*bp->cur = bp->restore;
606-
bp->restore = -1;
607+
*bp->cur = bp->save;
608+
bp->saving = 0;
607609
}
608610
r = bp->cur;
609611
*bp->end = 0;
@@ -631,10 +633,10 @@ masterline(Sfio_t* mp, Sfio_t* lp, char* prompt, int must, int timeout, Master_t
631633
{
632634
if (bp->cur < bp->end)
633635
{
634-
if (bp->restore >= 0)
636+
if (bp->saving)
635637
{
636-
*bp->cur = bp->restore;
637-
bp->restore = -1;
638+
*bp->cur = bp->save;
639+
bp->saving = 0;
638640
}
639641
r = bp->cur;
640642
*bp->end = 0;
@@ -669,11 +671,12 @@ masterline(Sfio_t* mp, Sfio_t* lp, char* prompt, int must, int timeout, Master_t
669671
}
670672
memcpy(bp->end, s, (size_t)n);
671673
bp->end += n;
672-
if ((r = bp->cur) > bp->buf && bp->restore >= 0)
673-
*r = bp->restore;
674+
if ((r = bp->cur) > bp->buf && bp->saving)
675+
*r = bp->save;
674676
if (bp->cur = memchr(bp->cur, '\n', (size_t)(bp->end - bp->cur)))
675677
{
676-
bp->restore = *++bp->cur;
678+
bp->save = *++bp->cur;
679+
bp->saving = 1;
677680
*bp->cur = 0;
678681
if (bp->cur >= bp->end)
679682
{
@@ -687,7 +690,7 @@ masterline(Sfio_t* mp, Sfio_t* lp, char* prompt, int must, int timeout, Master_t
687690
}
688691
else
689692
{
690-
bp->restore = -1;
693+
bp->saving = 0;
691694
bp->cur = r;
692695
bp->nxt = 0;
693696
must = 0;
@@ -803,7 +806,7 @@ dialogue(Sfio_t* mp, Sfio_t* lp, useconds_t delay, int timeout)
803806
master->buf = master->bufunderflow + BUFUNDERFLOW;
804807
master->cur = master->end = master->buf;
805808
master->max = master->buf + 2 * SFIO_BUFSIZE - 1;
806-
master->restore = -1;
809+
master->saving = 0;
807810
errno = 0;
808811
id = error_info.id;
809812
error_info.id = 0;

0 commit comments

Comments
 (0)