NetBSD Problem Report #53996

From gson@gson.org  Wed Feb 20 17:21:03 2019
Return-Path: <gson@gson.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-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 5C17B7A19B
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 20 Feb 2019 17:21:03 +0000 (UTC)
Message-Id: <20190220171917.243E098A91B@guava.gson.org>
Date: Wed, 20 Feb 2019 19:19:17 +0200 (EET)
From: gson@gson.org (Andreas Gustafsson)
Reply-To: gson@gson.org (Andreas Gustafsson)
To: gnats-bugs@NetBSD.org
Subject: tip(1) fails to handle EOF on stdin
X-Send-Pr-Version: 3.95

>Number:         53996
>Category:       bin
>Synopsis:       tip(1) fails to handle local EOF
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Feb 20 17:25:00 +0000 2019
>Closed-Date:    Thu Feb 28 17:47:37 +0000 2019
>Last-Modified:  Thu Feb 28 17:47:37 +0000 2019
>Originator:     Andreas Gustafsson
>Release:        NetBSD 8.0
>Organization:

>Environment:
System: NetBSD
Architecture: x86_64
Machine: amd64
>Description:

If the standard input to tip(1) ends (as in end-of-file, i.e. read()
returning 0), it fails to detect getchar() returning -1 and instead
interprets it as character with the value 0xFF, causing it to send
an endless stream of 0xFF characters (or possibly 0x7F depending on
parity settings) to the remote system.

>How-To-Repeat:

Noticed while running tip with a pty as stdin/stdout using pexpect
(devel/py-pexpect in pkgsrc) and running ktruss -p on it while
debugging an unrelated issue.  Also obvious from inspection of the
getchar() call in tipin().

>Fix:

>Release-Note:

>Audit-Trail:
From: Valery Ushakov <uwe@stderr.spb.ru>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/53996: tip(1) fails to handle EOF on stdin
Date: Fri, 22 Feb 2019 15:40:38 +0300

 On Wed, Feb 20, 2019 at 17:25:00 +0000, Andreas Gustafsson wrote:

 > >How-To-Repeat:
 > 
 > Noticed while running tip with a pty as stdin/stdout using pexpect
 > (devel/py-pexpect in pkgsrc) 

 Please, can you provide a minimized test case (p/expect script or
 something like that that).

 -uwe

From: Andreas Gustafsson <gson@gson.org>
To: Valery Ushakov <uwe@stderr.spb.ru>
Cc: gnats-bugs@NetBSD.org
Subject: Re: bin/53996: tip(1) fails to handle EOF on stdin
Date: Fri, 22 Feb 2019 16:14:02 +0200

 Valery Ushakov wrote:
 >  Please, can you provide a minimized test case (p/expect script or
 >  something like that that).

 To what end?  The bug is obvious on inspection and it will take less
 time to fix it than to construct a minimized test case for.  Thus,
 if I decide to spend more time on it, I'd rather just fix it.
 -- 
 Andreas Gustafsson, gson@gson.org

From: Valery Ushakov <uwe@stderr.spb.ru>
To: Andreas Gustafsson <gson@gson.org>
Cc: gnats-bugs@NetBSD.org
Subject: Re: bin/53996: tip(1) fails to handle EOF on stdin
Date: Fri, 22 Feb 2019 19:17:28 +0300

 On Fri, Feb 22, 2019 at 16:14:02 +0200, Andreas Gustafsson wrote:

 > Valery Ushakov wrote:
 > >  Please, can you provide a minimized test case (p/expect script or
 > >  something like that that).
 > 
 > To what end?  The bug is obvious on inspection and it will take less
 > time to fix it than to construct a minimized test case for.  Thus,
 > if I decide to spend more time on it, I'd rather just fix it.

 From your description it sounded like the minimized test case should
 be like a couple lines of python, like, in pseudocode

     spawn("tip")
     close_pty()

 and since you work with pexpect you presumably already know how these
 "spawn" and "close_pty" are really spelled, so it shouldn't be that
 much of a trouble.

 Is it more involved then that?

 -uwe

From: Andreas Gustafsson <gson@gson.org>
To: Valery Ushakov <uwe@stderr.spb.ru>
Cc: gnats-bugs@NetBSD.org
Subject: Re: bin/53996: tip(1) fails to handle EOF on stdin
Date: Fri, 22 Feb 2019 19:37:08 +0200

 Valery,

 You didn't answer my question: what purpose do you need the test case for?

 > >From your description it sounded like the minimized test case should
 > be like a couple lines of python, like, in pseudocode
 > 
 >     spawn("tip")
 >     close_pty()
 > 
 > and since you work with pexpect you presumably already know how these
 > "spawn" and "close_pty" are really spelled, so it shouldn't be that
 > much of a trouble.
 > 
 > Is it more involved then that?

 Yes, it is more involved than that.

 Assuming "dty0c" in your /etc/remote is pointing to a working serial
 port (which it happens to do on my system), the following test case
 should work.  Save this as test.py:

 import pexpect
 import sys
 import time
 import os
 child = pexpect.spawn("sh", ["-c", "trap '' HUP && ktrace tip dty0c"], logfile = sys.stdout)
 child.expect("onnected");
 time.sleep(1)
 os.close(child.child_fd)
 time.sleep(1)

 Then run the following as root:

 python2.7 test.py
 kdump

 kdump will show an a large number of system calls writing 0xFF
 characters to the serial port:

  19467      1 tip      CALL  write(3,0x7f7fffffdc0f,1)
  19467      1 tip      GIO   fd 3 wrote 1 bytes
        "\M^?"
  19467      1 tip      RET   write 1
  19467      1 tip      CALL  write(3,0x7f7fffffdc0f,1)
  19467      1 tip      GIO   fd 3 wrote 1 bytes
        "\M^?"
  19467      1 tip      RET   write 1

 and ktrace.out will keep growing until you kill the tip
 processes.
 -- 
 Andreas Gustafsson, gson@gson.org

From: "Valeriy E. Ushakov" <uwe@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/53996 CVS commit: src/usr.bin/tip
Date: Fri, 22 Feb 2019 22:25:22 +0000

 Module Name:	src
 Committed By:	uwe
 Date:		Fri Feb 22 22:25:22 UTC 2019

 Modified Files:
 	src/usr.bin/tip: tip.c

 Log Message:
 Check getchar() result for EOF.

 Call cleanup(SIGHUP) if we get local EOF, as if we've got SIGHUP.
 While here, use EOF constant instead of literal -1 in an existing
 check.

 PR bin/53996


 To generate a diff of this commit:
 cvs rdiff -u -r1.60 -r1.61 src/usr.bin/tip/tip.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Valery Ushakov <uwe@stderr.spb.ru>
To: Andreas Gustafsson <gson@gson.org>
Cc: gnats-bugs@NetBSD.org
Subject: Re: bin/53996: tip(1) fails to handle EOF on stdin
Date: Sat, 23 Feb 2019 01:28:35 +0300

 On Fri, Feb 22, 2019 at 19:37:08 +0200, Andreas Gustafsson wrote:

 > You didn't answer my question: what purpose do you need the test
 > case for?

 To reproduce a bug / test a fix, what else do you usually need test
 cases for :)


 > > Is it more involved then that?
 > 
 > Yes, it is more involved than that.
 [...]

 Thanks for the test.  This should hopefully be fixed now.  Let me know
 if I can request the pull-ups.


 -uwe

From: Andreas Gustafsson <gson@gson.org>
To: Valery Ushakov <uwe@stderr.spb.ru>
Cc: gnats-bugs@NetBSD.org
Subject: Re: bin/53996: tip(1) fails to handle EOF on stdin
Date: Sat, 23 Feb 2019 12:25:29 +0200

 Valery Ushakov wrote:
 > To reproduce a bug / test a fix, what else do you usually need test
 > cases for :)

 Calling getchar() without checking for EOF is always a bug, whether or
 not it can be demonstrated with a test case, and it also seems highly
 unlikely that adding the check for EOF would fail to fix the bug.
 But the important thing is that it's fixed - thanks!

 I do have a couple of style nits, though...

 The cast to char in in "return (char)c & STRIP_PAR;" is pointless;
 the result is immediately promoted back to int anyway for the "&"
 operation, and then converted to char again for the return.

 Also, I think it would be cleaner for xgetchar() to have the same
 return type as getchar().  It doesn't matter to the existing callers
 in tip, but in general, having a getchar() wrapper return char is bad
 because it will then return negative values for characters with the
 8th bit set on systems where char is signed, whereas getchar() itself
 will not.

 That is, I propose the following changes:

 Index: tip.c
 ===================================================================
 RCS file: /bracket/repo/src/usr.bin/tip/tip.c,v
 retrieving revision 1.61
 diff -u -r1.61 tip.c
 --- tip.c	22 Feb 2019 22:25:22 -0000	1.61
 +++ tip.c	23 Feb 2019 10:17:30 -0000
 @@ -332,7 +332,7 @@
  /*
   * getchar() wrapper that checks for EOF on the local end.
   */
 -static char
 +static int
  xgetchar(void)
  {
  	int c = getchar();
 @@ -341,7 +341,7 @@
  		/* NOTREACHED */
  	}

 -	return (char)c & STRIP_PAR;
 +	return c & STRIP_PAR;
  }

 > Let me know if I can request the pull-ups.

 Fine by me (with or without the above patch).
 -- 
 Andreas Gustafsson, gson@gson.org

State-Changed-From-To: open->closed
State-Changed-By: gson@NetBSD.org
State-Changed-When: Thu, 28 Feb 2019 17:47:37 +0000
State-Changed-Why:
Fixed by uwe.  I don't see a need for pullups given that no one else
has reported this in >20 years, but feel free to request them if you
disagree.


>Unformatted:

Home	PR Database Search