tcflush() is implemented incorrectly

[waddlesplash]

​https://xref.landonf.org/source/xref/haiku/src/system/libroot/posix/termios.c#88

​https://xref.landonf.org/source/xref/haiku/src/add-ons/kernel/generic/tty/tty.cpp#1774

The libroot version passes a value, the kernel counterpart expects a pointer.

FWIW, Linux apparently uses a value here, so if any code from other OSes invokes this ioctl directly, we need to go that route: ​https://github.com/bminor/musl/blob/cfdfd5ea3ce14c6abf7fb22a531f3d99518b5a1b/src/termios/tcflush.c

Oddly enough the TTY driver doesn't seem to support this ioctl at all however.

[bipolar]

tcdrain() seems to have a similar issue. I got an strace that reads:

[ 14335] ioctl(0x3, TCSBRK, 0x1, 0x0) = 0x80001301 Bad address () (52 us)

See the TCSBRK case on: ​​https://xref.landonf.org/source/xref/haiku/src/add-ons/kernel/generic/tty/tty.cpp#1753

[korli]

TCSBRK and TCFLSH are the only ones with int arg at ​https://man7.org/linux/man-pages/man4/tty_ioctl.4.html that we support.

[pulkomandy]

Oddly enough the TTY driver doesn't seem to support this ioctl at all however.

The tty driver is used only for pseudo-tty (Terminal) and should probably be renamed "pty" as in Linux. I don't think it makes sense to flush there as there are no buffers?

The tcflush support was added (by me) somewhat recently and I was not able to test it very much at the time. As said in the commit message (hrev54559) I have no idea what I'm doing here. I have just copied the code for other ioctls (that use a pointer) and didn't notice the difference.

I think rudolfc had done some testing and it at least allowed him to compile his app, but it wasn't working there either.

I think the "flush" ioctl also need implementation in at least pc_serial, possibly also usb_serial, to make sure the physical buffers are also flushed.

[bipolar]

Hello guys. After I made the following changes, I was able to use a Python module (PySerial, reading from real hardware with pc_serial on a loopback wire) without any modifications (it relies on termios' calls to tcflush/tcdrain/etc).

diff --git a/src/add-ons/kernel/generic/tty/tty.cpp b/src/add-ons/kernel/generic/tty/tty.cpp
index 913e2708..629003d0 100644
--- a/src/add-ons/kernel/generic/tty/tty.cpp
+++ b/src/add-ons/kernel/generic/tty/tty.cpp
@@ -1758,10 +1758,7 @@ tty_control(tty_cookie* cookie, uint32 op, void* buffer, size_t length)
                        else if (op == TIOCCBRK)
                                set = false;
                        else {
-                               int value;
-                               if (user_memcpy(&value, buffer, sizeof(value)) != B_OK)
-                                       return B_BAD_ADDRESS;
-
+                               uintptr_t value = (uintptr_t) buffer;
                                set = value != 0;
                        }
 
@@ -1773,9 +1770,7 @@ tty_control(tty_cookie* cookie, uint32 op, void* buffer, size_t length)
 
                case TCFLSH:
                {
-                       int value;
-                       if (user_memcpy(&value, buffer, sizeof(value)) != B_OK)
-                               return B_BAD_ADDRESS;
+                       uintptr_t value = (uintptr_t) buffer;
                        if (value & TCOFLUSH) {
                                struct tty* otherTTY = cookie->other_tty;
                                if (otherTTY->open_count <= 0)

I've tested the change on the latest nightlies, both on 32 and 64 bits. It works with the test case I'll attach after this comment.

I would like to submit the change for review, but I though it might be prudent to ask for input here first (as I feel I'm way out of my comfort zone with this types of changes).

Thanks in advance.

[korli]

an int value is preferred at least for TCFLSH (see TTYFLUSH ​here)

int value = (int)(uintptr_t) buffer;

[bipolar]

Replying to korli:

an int value is preferred at least for TCFLSH

Thanks korli!

I'll double check for TIOCCBRK too, and hopefully send a patch for review! Thank you for your work! It is always appreciated!

Edit: ​https://review.haiku-os.org/c/haiku/+/5545

[waddlesplash]

Fix merged.

[bipolar]

Updating the test script, as this version works more reliably and it's easier to use.