Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
158 changes: 121 additions & 37 deletions tty/libtty/libtty.c
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@

/* } DEBUG */

/* NOT supported: IXON|IXOFF|IXANY|PARMRK|INPCK|IGNPAR */
#define TTYSUP_IFLAG (IGNBRK | BRKINT | ISTRIP | INLCR | IGNCR | ICRNL | IMAXBEL)
#define TTYSUP_IFLAG (IGNBRK | BRKINT | ISTRIP | INLCR | IGNCR | ICRNL | IMAXBEL | IXON | IXOFF | IXANY | PARMRK | INPCK | IGNPAR)

#define TTYSUP_OFLAG (OPOST | ONLCR | TAB3 | OCRNL | ONOCR | ONLRET)
/* NOT supported: TOSTOP|FLUSHO|NOFLSH|ECHOPRT */
Expand All @@ -65,6 +64,8 @@
tty->cb.cb_name(tty->cb.arg, ##__VA_ARGS__); \
} while (0)

#define LIBTTY_IS_CLOSING(tty) ((atomic_load_explicit(&(tty)->t_flags, memory_order_relaxed) & TF_CLOSING) != 0)

#if 0
#define DEBUG_CHAR(c) \
do { \
Expand All @@ -89,10 +90,10 @@ static void termios_optimize(libtty_common_t *tty)
tty->breakchars[n] = '\0';

/* check if we have break char in the RX FIFO */
tty->t_flags &= ~TF_HAVEBREAK;
atomic_fetch_and_explicit(&tty->t_flags, ~TF_HAVEBREAK, memory_order_relaxed);
if (CMP_FLAG(l, ICANON)) {
if (libttydisc_rx_have_breakchar(tty))
tty->t_flags |= TF_HAVEBREAK;
atomic_fetch_or_explicit(&tty->t_flags, TF_HAVEBREAK, memory_order_relaxed);
}
}

Expand Down Expand Up @@ -147,34 +148,41 @@ static void termios_print_flags(const struct termios *termios_p)
}


ssize_t libtty_read(libtty_common_t *tty, char *data, size_t size, unsigned mode)
ssize_t libtty_read_helper(libtty_common_t *tty, char *data, size_t size, unsigned mode, libtty_read_state_t *st)
{
ssize_t ret = 0;

if (tty->t_flags & TF_CLOSING)
if (LIBTTY_IS_CLOSING(tty))
return -EBADF;

if (CMP_FLAG(l, ICANON))
ret = libttydisc_read_canonical(tty, data, size, mode, NULL);
ret = libttydisc_read_canonical(tty, data, size, mode, st);
else
ret = libttydisc_read_raw(tty, data, size, mode, NULL);
ret = libttydisc_read_raw(tty, data, size, mode, st);

/* IXOFF: send CSTART if we stopped the remote and the queue has space again */
if (CMP_FLAG(i, IXOFF)) {
if (fifo_freespace(tty->rx_fifo) >= LIBTTYDISC_INPUT_ON_THRESHOLD &&
(atomic_load_explicit(&tty->t_flags, memory_order_relaxed) & TF_IOFF) != 0) {
const char cstart = CSTART;
/* non-blocking write so that we don't deadlock */
libtty_write(tty, &cstart, 1, O_NONBLOCK);
atomic_fetch_and_explicit(&tty->t_flags, ~TF_IOFF, memory_order_relaxed);
}
}
Comment thread
julianuziemblo marked this conversation as resolved.
Comment on lines +163 to +172

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling fifo_freespace(tty->rx_fifo) without holding tty->rx_mutex is a data race because the RX FIFO can be concurrently modified by the interrupt worker thread or other contexts. To avoid this race and prevent potential lock inversion (since libtty_write locks tx_mutex), we should lock and unlock rx_mutex strictly around the fifo_freespace check.

	/* IXOFF: send CSTART if we stopped the remote and the queue has space again */
	if (CMP_FLAG(i, IXOFF)) {
		mutexLock(tty->rx_mutex);
		int space = fifo_freespace(tty->rx_fifo);
		mutexUnlock(tty->rx_mutex);
		if (space >= LIBTTYDISC_INPUT_ON_THRESHOLD &&
				(atomic_load_explicit(&tty->t_flags, memory_order_relaxed) & TF_IOFF) != 0) {
			const char cstart = CSTART;
			/* non-blocking write so that we don't deadlock */
			libtty_write(tty, &cstart, 1, O_NONBLOCK);
			atomic_fetch_and_explicit(&tty->t_flags, ~TF_IOFF, memory_order_relaxed);
		}
	}


return ret;
}

ssize_t libtty_read_nonblock(libtty_common_t *tty, char *data, size_t size, unsigned mode, libtty_read_state_t *st)
{
ssize_t ret = 0;

if (tty->t_flags & TF_CLOSING)
return -EBADF;

if (CMP_FLAG(l, ICANON))
ret = libttydisc_read_canonical(tty, data, size, mode, st);
else
ret = libttydisc_read_raw(tty, data, size, mode, st);
ssize_t libtty_read(libtty_common_t *tty, char *data, size_t size, unsigned mode)
{
return libtty_read_helper(tty, data, size, mode, NULL);
}

return ret;
ssize_t libtty_read_nonblock(libtty_common_t *tty, char *data, size_t size, unsigned mode, libtty_read_state_t *st)
{
return libtty_read_helper(tty, data, size, mode, st);
}


Expand Down Expand Up @@ -258,15 +266,11 @@ int libtty_init(libtty_common_t *tty, libtty_callbacks_t *callbacks, unsigned in

int libtty_close(libtty_common_t *tty)
{
mutexLock2(tty->tx_mutex, tty->rx_mutex);
tty->t_flags |= TF_CLOSING;
atomic_fetch_or_explicit(&tty->t_flags, TF_CLOSING, memory_order_relaxed);

condBroadcast(tty->tx_waitq);
condBroadcast(tty->rx_waitq);

mutexUnlock(tty->tx_mutex);
mutexUnlock(tty->rx_mutex);

return 0;
}
Comment thread
julianuziemblo marked this conversation as resolved.
Comment thread
julianuziemblo marked this conversation as resolved.
Comment thread
julianuziemblo marked this conversation as resolved.

Expand All @@ -291,7 +295,7 @@ ssize_t libtty_write(libtty_common_t *tty, const char *data, size_t size, unsign
ssize_t len = 0;

/* short path */
if (tty->t_flags & TF_CLOSING)
if (LIBTTY_IS_CLOSING(tty))
return -EPIPE;
else if (fifo_is_full(tty->tx_fifo) && (mode & O_NONBLOCK))
return -EWOULDBLOCK;
Expand All @@ -305,13 +309,15 @@ ssize_t libtty_write(libtty_common_t *tty, const char *data, size_t size, unsign
/* write contents of the buffer */
while (len < size) {
while (fifo_freespace(tty->tx_fifo) < fifo_freespace_for_single_char) {
if (tty->t_flags & TF_CLOSING)
if (LIBTTY_IS_CLOSING(tty))
goto exit;

if (mode & O_NONBLOCK)
goto exit;

CALLBACK(signal_txready);
if ((atomic_load_explicit(&tty->t_flags, memory_order_relaxed) & TF_OOFF) == 0) {
CALLBACK(signal_txready);
}
condWait(tty->tx_waitq, tty->tx_mutex, 0);
}

Expand All @@ -327,16 +333,18 @@ ssize_t libtty_write(libtty_common_t *tty, const char *data, size_t size, unsign
}

/* DEBUG_CHAR('W'); */
CALLBACK(signal_txready);
if ((atomic_load_explicit(&tty->t_flags, memory_order_relaxed) & TF_OOFF) == 0) {
CALLBACK(signal_txready);
}
#if 0
/* TODO: test O_SYNC */
while ((mode & O_SYNC) && !fifo_is_empty(tty->tx_fifo) && !(tty->t_flags & TF_CLOSING))
while ((mode & O_SYNC) && !fifo_is_empty(tty->tx_fifo) && !(LIBTTY_IS_CLOSING(tty)))
condWait(tty->tx_waitq, tty->tx_mutex, 0);
#endif

exit:

if (tty->t_flags & TF_CLOSING)
if (LIBTTY_IS_CLOSING(tty))
len = -EPIPE;
else if ((len == 0) && (mode & O_NONBLOCK))
len = -EWOULDBLOCK;
Expand All @@ -356,7 +364,7 @@ int libtty_txready(libtty_common_t *tty)
DEBUG_CHAR('F');
#endif

return !fifo_is_empty(tty->tx_fifo);
return ((atomic_load_explicit(&tty->t_flags, memory_order_relaxed) & TF_OOFF) != 0) ? 0 : !fifo_is_empty(tty->tx_fifo);
}

int libtty_txfull(libtty_common_t *tty)
Expand All @@ -379,13 +387,13 @@ int libtty_poll_status(libtty_common_t *tty)
revents |= POLLIN | POLLRDNORM;
}
else {
if (tty->t_flags & TF_HAVEBREAK)
if ((atomic_load_explicit(&tty->t_flags, memory_order_relaxed) & TF_HAVEBREAK) != 0)
revents |= POLLIN | POLLRDNORM;
}

if (!libtty_txfull(tty))
revents |= POLLOUT | POLLWRNORM;
if (tty->t_flags & TF_CLOSING)
if (LIBTTY_IS_CLOSING(tty))
revents |= POLLHUP;

return revents;
Expand All @@ -407,6 +415,68 @@ void libtty_drain(libtty_common_t *tty)

mutexUnlock(tty->tx_mutex);
}

static int libtty_flow(libtty_common_t *tty, int action)
{
int ret = 0;
char data[1];

switch (action) {
case TCOOFF:
atomic_fetch_or_explicit(&tty->t_flags, TF_OOFF, memory_order_relaxed);
break;

case TCOON:
atomic_fetch_and_explicit(&tty->t_flags, ~TF_OOFF, memory_order_relaxed);
condBroadcast(tty->tx_waitq);

mutexLock(tty->tx_mutex);
CALLBACK(signal_txready);
mutexUnlock(tty->tx_mutex);
Comment thread
julianuziemblo marked this conversation as resolved.
break;

case TCIOFF:
data[0] = CSTOP;
ret = libtty_write(tty, data, 1, O_NONBLOCK);
if (ret > 0) {
ret = 0;
}
break;

case TCION:
data[0] = CSTART;
ret = libtty_write(tty, data, 1, O_NONBLOCK);
if (ret > 0) {
ret = 0;
}
break;
Comment thread
julianuziemblo marked this conversation as resolved.
Comment thread
julianuziemblo marked this conversation as resolved.

default:
ret = -EINVAL;
break;
}
Comment thread
julianuziemblo marked this conversation as resolved.

return ret;
}

static int libtty_sendbreak(libtty_common_t *tty, int duration)
{
if (duration < 0) {
return -EINVAL;
}

if (tty->cb.break_enable == NULL) {
return -EOPNOTSUPP;
}

time_t durationUs = ((duration == 0) ? 250 : duration) * 1000;
CALLBACK(break_enable, true);
usleep(durationUs);
CALLBACK(break_enable, false);

return 0;
}

void libtty_flush(libtty_common_t *tty, int type)
{
if (type == TCIFLUSH || type == TCIOFLUSH) {
Expand All @@ -432,6 +502,7 @@ int libtty_ioctl(libtty_common_t *tty, pid_t sender_pid, unsigned int cmd, const
struct termios *termios_p = (struct termios *)in_arg;
struct winsize *ws = (struct winsize *)in_arg;
pid_t *pid = (pid_t *)in_arg;
int inVal = (int)(uintptr_t)in_arg;
int ret = 0;

*out_arg = NULL;
Expand All @@ -454,14 +525,27 @@ int libtty_ioctl(libtty_common_t *tty, pid_t sender_pid, unsigned int cmd, const
log_ioctl("TCDRAIN");
libtty_drain(tty);
break;
case TCXONC:
log_ioctl("TCXONC (%d)", inVal);
ret = libtty_flow(tty, inVal);
break;
case TCSBRK:
log_ioctl("TCSBRK (%d)", inVal);
ret = libtty_sendbreak(tty, inVal);
break;
Comment thread
julianuziemblo marked this conversation as resolved.
case TCFLSH:
log_ioctl("TCFLSH");
/* WARN: passing ioctl attr by value */
libtty_flush(tty, (long)in_arg);
libtty_flush(tty, inVal);
break;
case TCSETS:

case TCSETSF:
libtty_flush(tty, TCIFLUSH);
/* fallthrough */
case TCSETSW:
case TCSETSF: {
libtty_drain(tty);
/* fallthrough */
case TCSETS: {
Comment on lines +542 to +548

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In TCSETSF, the input queue is flushed before draining the output queue. Since draining the output queue (libtty_drain) blocks and can take time, new input characters could arrive during this period and would not be flushed. To adhere to the standard behavior where all input received up to the point of the termios change is discarded, the output queue should be drained first, followed by flushing the input queue.

Suggested change
case TCSETSF:
libtty_flush(tty, TCIFLUSH);
/* fallthrough */
case TCSETSW:
case TCSETSF: {
libtty_drain(tty);
/* fallthrough */
case TCSETS: {
case TCSETSF:
libtty_drain(tty);
libtty_flush(tty, TCIFLUSH);
goto set_termios;
case TCSETSW:
libtty_drain(tty);
/* fallthrough */
case TCSETS:
set_termios: {

log_ioctl("TCSETS (%s)", ((termios_p->c_lflag & ICANON) ? "cooked" : "raw"));
/* TODO: SW SF */

Expand Down Expand Up @@ -533,7 +617,7 @@ int libtty_ioctl(libtty_common_t *tty, pid_t sender_pid, unsigned int cmd, const
return -EIO;
}
/* WARN: passing ioctl half-duplex enable by value */
int enable = (int)(uintptr_t)in_arg;
int enable = inVal;
log_ioctl("TIOCSHALFD: enable = %d", enable);
if ((enable != 0) && (enable != 1)) {
log_warn("halfduplex enable (%d) != {0,1}", enable);
Expand Down
20 changes: 17 additions & 3 deletions tty/libtty/libtty.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#ifndef _LIBTTY_H_
#define _LIBTTY_H_

#include <stdatomic.h>
#include <stdbool.h>
#include <stdint.h>
#include <termios.h>
#include <unistd.h>
Expand All @@ -40,6 +42,9 @@ struct libtty_callbacks_s {

/* at least one character ready to be sent */
void (*signal_txready)(void *arg);

/* enable/disable break condition */
void (*break_enable)(void *arg, bool enable);
};

struct libtty_common_s {
Expand All @@ -61,7 +66,7 @@ struct libtty_common_s {

/* cached optimizations */
char breakchars[4]; /* enough to hold \n, VEOF and VEOL. */
unsigned int t_flags;
atomic_uint t_flags;

/* TODO: remove */
volatile uint32_t *debug;
Expand All @@ -84,6 +89,15 @@ static inline void libtty_read_state_init(libtty_read_state_t *st)
#define TF_LITERAL 0x00200 /* Accept the next character literally. */
#define TF_BYPASS 0x04000 /* Optimized input path. */
#define TF_CLOSING 0x08000 /* TTY is being closed */
#define TF_OOFF 0x10000 /* TTY output is stopped */
#define TF_IOFF 0x20000 /* TTY input is stopped */


enum charflags {
cf_normal = 1U << 0,
cf_parity = 1U << 1,
cf_break = 1U << 2,
};


/* bufsize: TX/RX buffer size - has to be power of 2 ! */
Expand All @@ -108,11 +122,11 @@ int libtty_ioctl(libtty_common_t *tty, pid_t sender_pid, unsigned int cmd, const
ssize_t libtty_read_nonblock(libtty_common_t *tty, char *data, size_t size, unsigned mode, libtty_read_state_t *st);

/* internal (HW) interface */
int libtty_putchar(libtty_common_t *tty, unsigned char c, int *wake_reader);
int libtty_putchar(libtty_common_t *tty, unsigned char c, enum charflags cf, int *wake_reader);
void libtty_putchar_lock(libtty_common_t *tty);
void libtty_putchar_unlock(libtty_common_t *tty);
void libtty_wake_reader(libtty_common_t *tty);
int libtty_putchar_unlocked(libtty_common_t *tty, unsigned char c, int *wake_reader);
int libtty_putchar_unlocked(libtty_common_t *tty, unsigned char c, enum charflags cf, int *wake_reader);
/* writer wake up is done outside of libtty if wake_writer is not NULL */
unsigned char libtty_getchar(libtty_common_t *tty, int *wake_writer);
unsigned char libtty_popchar(libtty_common_t *tty);
Expand Down
Loading
Loading