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]

Message-ID: <87eiql5igv.fsf@devron.myhome.or.jp>
Date:	Sun, 06 Sep 2009 03:56:00 +0900
From:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Mikael Pettersson <mikpe@...uu.se>,
	"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>
Subject: Re: [Bug #14015] pty regressed again, breaking expect and gcc's testsuite

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Sun, 6 Sep 2009, OGAWA Hirofumi wrote:
>> 
>> This is not meaning to object to your patch though, I think we would be
>> good to fix pty_space(), not leaving as wrong. With fix it, I guess we
>> don't get strange behavior in the near of buffer limit.
>
> I'd actually rather not make that function any more complicated.
>
> Just make the rules be very simple:
>
>  - the pty layer has ~64kB buffering, and if you just blindly do a 
>    ->write() op, you can see how many characters you were able to write.
>
>  - before doing a ->write() op, you can ask how many characters you are 
>    guaranteed to be able to write by doing a "->write_room()" call.
>
> ..and then the bug literally was just that "pty_write()" was confused, and 
> thought that it should do that "write_room()" thing, which it really 
> shouldn't ever have done.
>
> So I really think that the true fix is to just remove the code from 
> pty_write(), and not do anything more complicated. I'll also commit the 
> change to write '\r\n' as one single string, because quite frankly, it's 
> just stupid to do it as two characters, but at that point it's just a 
> cleanup.

But, current write_room() returns almost all wrong value. For example,
if we have the 4kb preallocated buffer in some state and used it,
->memory_used will be 4kb even if we are using only a byte actually.

I thought it's strange/wrong, even if we removed the pty_space() in
pty_write().

>> Also, it seems the non-n_tty path doesn't use tty_write_room() check,
>> and instead it just try to write and check written bytes which returned
>> by tty->ops->write().
>
> .. and I think that's fine. I think write_room() should be used sparingly, 
> and only by code that cares about being able to fit at least 'n' 
> characters in the tty buffers. In fact, I think even n_tty would likely in 
> general be better off without it (and just check the return value), but 
> because of the stateful character translation (that doesn't actually keep 
> any state around, it just wants to expand things as it goes along), and 
> because of historical reasons, we'll just keep it using write_room.

As a bit long term solution, I agree. Current code seems to have fragile
buffer handling about echoes, \n etc. And yes, perhaps, to avoid
write_room() is clean way.

But, I felt 64kb (pty_write) vs 8kb (pty_write_room) sounds strange
currently.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
--
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/