linux-kernel - Re: [Bug #14015] pty regressed again, breaking expect and gcc's testsuite

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 4 Sep 2009 08:11:50 -1000 (HST)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Mikael Pettersson <mikpe@...uu.se>
cc:	"Rafael J. Wysocki" <rjw@...k.pl>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Kernel Testers List <kernel-testers@...r.kernel.org>,
	Alan Cox <alan@...ux.intel.com>, Greg KH <gregkh@...e.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Subject: Re: [Bug #14015] pty regressed again, breaking expect and gcc's
 testsuite

On Fri, 4 Sep 2009, Linus Torvalds wrote:
> 
> And again - UNTESTED. Maybe this makes the buffering _too_ small (the 
> 'memory_used' thing is not really counted in bytes buffered, it's counted 
> in how much buffer space we've allocated) and things break even worse and 
> pty's don't work at all. But I think it might work.

Actually, scratch that patch.

After writing the above, the voices in my head started clamoring about 
this "space allocated" vs "bytes buffered" thing, which I was obviously 
aware of, but hadn't thought about as an issue.

And you know what? The thing about "space allocated" vs "bytes buffered" 
is that writing _one_ byte (the '\r') can cause a lot more than one byte 
to be allocated for a buffer (we do minimum 256-byte buffers).

So let's say that 'space' was initially 20 - plenty big enough to hold two 
characters. But if the '\r' just happened to need a new buffer, it would 
actually increase 'memory_used' by 256, and now the next time we call 
'pty_space()' it doesn't return 19, but 0 - because now memory_used is 
larger than the 8192 we allowed.

So I'm starting to suspect that the real bug is that we do that 
'pty_space()' in pty_write() call at all. The _callers_ should already 
have done the write_room() check, and if somebody doesn't do it, then the 
tty buffering will eventually do a hard limit at the 65kB allocation mark.

So doing it in pty_write() is (a) unnecessary and (b) actively wrong, 
because it means that in the situation above, pty_write() won't be allowin 
the slop that it _needs_ to allow due to the buffering not being exact 
"this many bytes buffered up", but "this many bytes allocated for 
buffering".

So rather than the previous patch, try this one instead.

		Linus
---
 drivers/char/pty.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/char/pty.c b/drivers/char/pty.c
index d083c73..45a7ca2 100644
--- a/drivers/char/pty.c
+++ b/drivers/char/pty.c
@@ -118,12 +118,6 @@ static int pty_write(struct tty_struct *tty, const unsigned char *buf,
 	if (tty->stopped)
 		return 0;

-	/* This isn't locked but our 8K is quite sloppy so no
-	   big deal */
-
-	c = pty_space(to);
-	if (c > count)
-		c = count;
 	if (c > 0) {
 		/* Stuff the data into the input queue of the other end */
 		c = tty_insert_flip_string(to, buf, c);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/