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

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [Bug #14015] pty regressed again, breaking expect and gccs testsuite
    ... This is not meaning to object to your patch though, ... good to fix pty_space, not leaving as wrong. ... don't get strange behavior in the near of buffer limit. ... you can see how many characters you were able to write. ...
    (Linux-Kernel)
  • [UNIX] wu-ftpd fb_realpath() Off-by-One Bug
    ... Wu-ftpd FTP server contains remotely exploitable off-by-one bug. ... characters while the size of the buffer is MAXPATHLEN characters only. ... Following FTP commands may be used to cause buffer overflow: ...
    (Securiteam)
  • Re: Serial Port CE_OVERRUN errors
    ... SerialNG component. ... The main Input buffer is set to 32000, and I have a variable which is ... updated with the maximum number of characters waiting there each time the ... I tried boosting the baud rate from 57.6k to 115.2k, ...
    (comp.lang.pascal.delphi.misc)
  • RE: [Full-disclosure] How many vendors knowingly ship GA product with security vulnerabilities?
    ... It would seem that if a few days buffer were built into the system, ... How many vendors knowingly ship GA ... But it can *still* take a while to actually integrate and test the fix, ... Find a 'struct' in that .h, ...
    (Full-Disclosure)
  • Re: WaitCommEvent and ResetEvent for SerialPort
    ... This is extremely true when you say you are exhausting the input buffer, ... This receiver thread will then signal your main ... >> From the WaitCommEvent documentation, ... Since no characters are currently coming it, ...
    (microsoft.public.win32.programmer.kernel)