From 9b1de7bbb27872384d63913e2ee3ba59cdaef0fa Mon Sep 17 00:00:00 2001 From: Robert Quattlebaum Date: Mon, 12 Sep 2016 13:47:18 -0700 Subject: [PATCH] posix/uart: Fixes for failure to terminate (#568) These changes address various issues found in the POSIX platform UART driver which lead to the process not terminating properly when the parent dies or closes `stdin`/`stdout`. Some of the errors could even lead to unexpected program termination. Among the various errors corrected are: * Using `assert()` to terminate under normal operating conditions. * `posixUartProcess()` only checked `s_in_fd` for readability and did not check `s_out_fd` for writability. It also caused data corruption if `write()` ever returned less than all of the bytes sent. * `ISIG` was being set on the termios flags. This can cause problems when the serial API is used for binary data (Like when used with the NCP app). (This requires the CLI be explicitly able to be able to handle CTRL-C when built for POSIX) * Socket errors weren't being tracked on `select()`. * The platform main loop would (possibly) terminate if `select()` failed with `EINTR`, which makes it difficult to attach to the process with a debugger. --- examples/platforms/posix/platform-posix.h | 2 +- examples/platforms/posix/platform.c | 17 ++- examples/platforms/posix/radio.c | 14 ++- examples/platforms/posix/uart.c | 125 +++++++++++++++++----- src/cli/cli_uart.cpp | 7 ++ 5 files changed, 129 insertions(+), 36 deletions(-) diff --git a/examples/platforms/posix/platform-posix.h b/examples/platforms/posix/platform-posix.h index 100713d43..3c9fbe1e7 100644 --- a/examples/platforms/posix/platform-posix.h +++ b/examples/platforms/posix/platform-posix.h @@ -107,7 +107,7 @@ void posixRandomInit(void); * @param[inout] aMaxFd A pointer to the max file descriptor. * */ -void posixUartUpdateFdSet(fd_set *aReadFdSet, fd_set *aWriteFdSet, int *aMaxFd); +void posixUartUpdateFdSet(fd_set *aReadFdSet, fd_set *aWriteFdSet, fd_set *aErrorFdSet, int *aMaxFd); /** * This function performs radio driver processing. diff --git a/examples/platforms/posix/platform.c b/examples/platforms/posix/platform.c index f986a1291..2d4bf2d0a 100644 --- a/examples/platforms/posix/platform.c +++ b/examples/platforms/posix/platform.c @@ -54,7 +54,7 @@ void PlatformInit(int argc, char *argv[]) if (argc != 2) { - exit(1); + exit(EXIT_FAILURE); } NODE_ID = (uint32_t)strtol(argv[1], &endptr, 0); @@ -62,7 +62,7 @@ void PlatformInit(int argc, char *argv[]) if (*endptr != '\0') { fprintf(stderr, "Invalid NODE_ID: %s\n", argv[1]); - exit(1); + exit(EXIT_FAILURE); } posixAlarmInit(); @@ -75,21 +75,28 @@ void PlatformProcessDrivers(void) { fd_set read_fds; fd_set write_fds; + fd_set error_fds; int max_fd = -1; struct timeval timeout; int rval; FD_ZERO(&read_fds); FD_ZERO(&write_fds); + FD_ZERO(&error_fds); - posixUartUpdateFdSet(&read_fds, &write_fds, &max_fd); + posixUartUpdateFdSet(&read_fds, &write_fds, &error_fds, &max_fd); posixRadioUpdateFdSet(&read_fds, &write_fds, &max_fd); posixAlarmUpdateTimeout(&timeout); if (!otAreTaskletsPending()) { - rval = select(max_fd + 1, &read_fds, &write_fds, NULL, &timeout); - assert(rval >= 0 && errno != ETIME); + rval = select(max_fd + 1, &read_fds, &write_fds, &error_fds, &timeout); + + if ((rval < 0) && (errno != EINTR)) + { + perror("select"); + exit(EXIT_FAILURE); + } } posixUartProcess(); diff --git a/examples/platforms/posix/radio.c b/examples/platforms/posix/radio.c index 9e1924730..a348bde93 100644 --- a/examples/platforms/posix/radio.c +++ b/examples/platforms/posix/radio.c @@ -446,7 +446,12 @@ bool otPlatRadioGetPromiscuous(void) void radioReceive(void) { ssize_t rval = recvfrom(sSockFd, &sReceiveMessage, sizeof(sReceiveMessage), 0, NULL, NULL); - assert(rval >= 0); + + if (rval < 0) + { + perror("recvfrom"); + exit(EXIT_FAILURE); + } sReceiveFrame.mLength = (uint8_t)(rval - 1); @@ -563,7 +568,12 @@ void radioTransmit(const struct RadioMessage *msg, const struct RadioPacket *pkt sockaddr.sin_port = htons(9000 + sPortOffset + i); rval = sendto(sSockFd, msg, 1 + pkt->mLength, 0, (struct sockaddr *)&sockaddr, sizeof(sockaddr)); - assert(rval >= 0); + + if (rval < 0) + { + perror("recvfrom"); + exit(EXIT_FAILURE); + } } } diff --git a/examples/platforms/posix/uart.c b/examples/platforms/posix/uart.c index 162ce569e..653015f05 100644 --- a/examples/platforms/posix/uart.c +++ b/examples/platforms/posix/uart.c @@ -33,12 +33,15 @@ #include #include #include +#include +#include #include #include #include "platform-posix.h" #ifdef OPENTHREAD_TARGET_LINUX +#include int posix_openpt(int oflag); int grantpt(int fildes); int unlockpt(int fd); @@ -69,10 +72,20 @@ ThreadError otPlatUartEnable(void) ThreadError error = kThreadError_None; struct termios termios; +#ifdef OPENTHREAD_TARGET_LINUX + // Ensure we terminate this process if the + // parent process dies. + prctl(PR_SET_PDEATHSIG, SIGHUP); +#endif + s_in_fd = dup(STDIN_FILENO); s_out_fd = dup(STDOUT_FILENO); dup2(STDERR_FILENO, STDOUT_FILENO); + // We need this signal to make sure that this + // process terminates properly. + signal(SIGPIPE, SIG_DFL); + if (isatty(s_in_fd)) { tcgetattr(s_in_fd, &original_stdin_termios); @@ -90,20 +103,17 @@ ThreadError otPlatUartEnable(void) // get current configuration VerifyOrExit(tcgetattr(s_in_fd, &termios) == 0, perror("tcgetattr"); error = kThreadError_Error); - // turn off input processing - termios.c_iflag &= ~(unsigned long)(IGNBRK | BRKINT | ICRNL | INLCR | PARMRK | INPCK | ISTRIP | IXON); + // Set up the termios settings for raw mode. This turns + // off input/output processing, line processing, and character processing. + cfmakeraw(&termios); - // turn off line processing - termios.c_lflag &= ~(unsigned long)(ECHO | ECHONL | ICANON | IEXTEN); + // Set up our cflags for local use. Turn on hangup-on-close. + termios.c_cflag |= HUPCL | CREAD | CLOCAL; - // turn off character processing - termios.c_cflag &= ~(unsigned long)(CSIZE | PARENB); - termios.c_cflag |= CS8 | HUPCL | CREAD | CLOCAL; - - // return 1 byte at a time + // "Minimum number of characters for noncanonical read" termios.c_cc[VMIN] = 1; - // turn off inter-character timer + // "Timeout in deciseconds for noncanonical read" termios.c_cc[VTIME] = 0; // configure baud rate @@ -118,12 +128,15 @@ ThreadError otPlatUartEnable(void) // get current configuration VerifyOrExit(tcgetattr(s_out_fd, &termios) == 0, perror("tcgetattr"); error = kThreadError_Error); - // turn off output processing + // Set up the termios settings for raw mode. This turns + // off input/output processing, line processing, and character processing. + cfmakeraw(&termios); + + // Absolutely obliterate all output processing. termios.c_oflag = 0; - // turn off character processing - termios.c_cflag &= ~(unsigned long)(CSIZE | PARENB); - termios.c_cflag |= CS8 | HUPCL | CREAD | CLOCAL; + // Set up our cflags for local use. Turn on hangup-on-close. + termios.c_cflag |= HUPCL | CREAD | CLOCAL; // configure baud rate VerifyOrExit(cfsetospeed(&termios, B115200) == 0, perror("cfsetospeed"); error = kThreadError_Error); @@ -163,22 +176,32 @@ exit: return error; } -void posixUartUpdateFdSet(fd_set *aReadFdSet, fd_set *aWriteFdSet, int *aMaxFd) +void posixUartUpdateFdSet(fd_set *aReadFdSet, fd_set *aWriteFdSet, fd_set *aErrorFdSet, int *aMaxFd) { if (aReadFdSet != NULL) { FD_SET(s_in_fd, aReadFdSet); + if (aErrorFdSet != NULL) + { + FD_SET(s_in_fd, aErrorFdSet); + } + if (aMaxFd != NULL && *aMaxFd < s_in_fd) { *aMaxFd = s_in_fd; } } - if (aWriteFdSet != NULL && s_write_length > 0) + if ((aWriteFdSet != NULL) && (s_write_length > 0)) { FD_SET(s_out_fd, aWriteFdSet); + if (aErrorFdSet != NULL) + { + FD_SET(s_out_fd, aErrorFdSet); + } + if (aMaxFd != NULL && *aMaxFd < s_out_fd) { *aMaxFd = s_out_fd; @@ -188,22 +211,68 @@ void posixUartUpdateFdSet(fd_set *aReadFdSet, fd_set *aWriteFdSet, int *aMaxFd) void posixUartProcess(void) { - const int flags = POLLIN | POLLRDNORM | POLLERR | POLLNVAL | POLLHUP; - struct pollfd pollfd = { s_in_fd, flags, 0 }; ssize_t rval; - - if (poll(&pollfd, 1, 0) > 0 && (pollfd.revents & flags) != 0) + const int error_flags = POLLERR | POLLNVAL | POLLHUP; + struct pollfd pollfd[] = { - rval = read(s_in_fd, s_receive_buffer, sizeof(s_receive_buffer)); - assert(rval >= 0); - otPlatUartReceived(s_receive_buffer, (uint16_t)rval); + { s_in_fd, POLLIN | error_flags, 0 }, + { s_out_fd, POLLOUT | error_flags, 0 }, + }; + + errno = 0; + + rval = poll(pollfd, sizeof(pollfd) / sizeof(*pollfd), 0); + + if (rval < 0) + { + perror("poll"); + exit(EXIT_FAILURE); } - if (s_write_length > 0) + if (rval > 0) { - rval = write(s_out_fd, s_write_buffer, s_write_length); - assert(rval >= 0); - s_write_length = 0; - otPlatUartSendDone(); + if ((pollfd[0].revents & error_flags) != 0) + { + perror("s_in_fd"); + exit(EXIT_FAILURE); + } + + if ((pollfd[1].revents & error_flags) != 0) + { + perror("s_out_fd"); + exit(EXIT_FAILURE); + } + + if (pollfd[0].revents & POLLIN) + { + rval = read(s_in_fd, s_receive_buffer, sizeof(s_receive_buffer)); + + if (rval <= 0) + { + perror("read"); + exit(EXIT_FAILURE); + } + + otPlatUartReceived(s_receive_buffer, (uint16_t)rval); + } + + if ((s_write_length > 0) && (pollfd[1].revents & POLLOUT)) + { + rval = write(s_out_fd, s_write_buffer, s_write_length); + + if (rval <= 0) + { + perror("write"); + exit(EXIT_FAILURE); + } + + s_write_buffer += (uint16_t)rval; + s_write_length -= (uint16_t)rval; + + if (s_write_length == 0) + { + otPlatUartSendDone(); + } + } } } diff --git a/src/cli/cli_uart.cpp b/src/cli/cli_uart.cpp index ee2e74cfa..7a708278d 100644 --- a/src/cli/cli_uart.cpp +++ b/src/cli/cli_uart.cpp @@ -98,6 +98,13 @@ void Uart::ReceiveTask(const uint8_t *aBuf, uint16_t aBufLength) break; +#ifdef OPENTHREAD_EXAMPLES_POSIX + + case 0x03: // ASCII for Ctrl-C + exit(EXIT_SUCCESS); + break; +#endif + case '\b': case 127: if (mRxLength > 0)