io_callback function

Hi,

I'm testing your code and I encounter a problem. "What" parameter can be "SUDO_EV_READ" or "SUDO_EV_WRITE" which are represented as second or third bit of integer. If "what" parameter will be  "SUDO_EV_READ" and "SUDO_EV_WRITE" at the same time than in IF(what, SUDO_EV_READ) fd will close and in IF(what, SUDO_EV_WRITE) is called "write" function with invalid file descriptor.

Code is here:

static void
io_callback(int fd, int what, void *v)
{
    struct io_buffer *iob = v;
    struct sudo_event_base *evbase;
    int n;
    debug_decl(io_callback, SUDO_DEBUG_EXEC);

    assert(!(ISSET(what, SUDO_EV_READ) && ISSET(what, SUDO_EV_WRITE)));

    if (ISSET(what, SUDO_EV_READ)) {
	evbase = sudo_ev_get_base(iob->revent);
	do {
	    n = read(fd, iob->buf + iob->len, sizeof(iob->buf) - iob->len);
	} while (n == -1 && errno == EINTR);
	switch (n) {
	    case -1:
		if (errno == EAGAIN)
		    break;
		/* treat read error as fatal and close the fd */
		sudo_debug_printf(SUDO_DEBUG_ERROR,
		    "error reading fd %d: %s", fd, strerror(errno));
		/* FALLTHROUGH */
	    case 0:
		/* got EOF or pty has gone away */
		if (n == 0) {
		    sudo_debug_printf(SUDO_DEBUG_INFO,
			"read EOF from fd %d", fd);
		}
		safe_close(fd);
		ev_free_by_fd(evbase, fd);
		/* If writer already consumed the buffer, close it too. */
		if (iob->wevent != NULL && iob->off == iob->len) {
		    safe_close(sudo_ev_get_fd(iob->wevent));
		    ev_free_by_fd(evbase, sudo_ev_get_fd(iob->wevent));
		    iob->off = iob->len = 0;
		}
		break;
	    default:
		sudo_debug_printf(SUDO_DEBUG_INFO,
		    "read %d bytes from fd %d", n, fd);
		if (!iob->action(iob->buf + iob->len, n, iob))
		    terminate_command(cmnd_pid, true);
		iob->len += n;
		/* Enable writer if not /dev/tty or we are foreground pgrp. */
		if (iob->wevent != NULL &&
		    (foreground || !USERTTY_EVENT(iob->wevent))) {
		    if (sudo_ev_add(evbase, iob->wevent, NULL, false) == -1)
			sudo_fatal(U_("unable to add event to queue"));
		}
		/* Re-enable reader if buffer is not full. */
		if (iob->len != sizeof(iob->buf)) {
		    if (sudo_ev_add(evbase, iob->revent, NULL, false) == -1)
			sudo_fatal(U_("unable to add event to queue"));
		}
		break;
	}
    }
    if (ISSET(what, SUDO_EV_WRITE)) {
	evbase = sudo_ev_get_base(iob->wevent);
	do {
	    n = write(fd, iob->buf + iob->off, iob->len - iob->off);
	} while (n == -1 && errno == EINTR);
	if (n == -1) {
	    switch (errno) {
	    case EPIPE:
	    case ENXIO:
	    case EIO:
	    case EBADF:
		/* other end of pipe closed or pty revoked */
		sudo_debug_printf(SUDO_DEBUG_INFO,
		    "unable to write %d bytes to fd %d",
		    iob->len - iob->off, fd);
		if (iob->revent != NULL) {
		    safe_close(sudo_ev_get_fd(iob->revent));
		    ev_free_by_fd(evbase, sudo_ev_get_fd(iob->revent));
		}
		safe_close(fd);
		ev_free_by_fd(evbase, fd);
		break;
	    case EAGAIN:
		/* not an error */
		break;
	    default:
#if 0 /* XXX -- how to set cstat? stash in iobufs instead? */
		if (cstat != NULL) {
		    cstat->type = CMD_ERRNO;
		    cstat->val = errno;
		}
#endif /* XXX */
		sudo_debug_printf(SUDO_DEBUG_ERROR,
		    "error writing fd %d: %s", fd, strerror(errno));
		sudo_ev_loopbreak(evbase);
		break;
	    }
	} else {
	    sudo_debug_printf(SUDO_DEBUG_INFO,
		"wrote %d bytes to fd %d", n, fd);
	    iob->off += n;
	    /* Reset buffer if fully consumed. */
	    if (iob->off == iob->len) {
		iob->off = iob->len = 0;
		/* Forward the EOF from reader to writer. */
		if (iob->revent == NULL) {
		    safe_close(fd);
		    ev_free_by_fd(evbase, fd);
		}
	    }
	    /* Re-enable writer if buffer is not empty. */
	    if (iob->len > iob->off) {
		if (sudo_ev_add(evbase, iob->wevent, NULL, false) == -1)
		    sudo_fatal(U_("unable to add event to queue"));
	    }
	    /* Enable reader if buffer is not full. */
	    if (iob->revent != NULL &&
		(ttymode == TERM_RAW || !USERTTY_EVENT(iob->revent))) {
		if (iob->len != sizeof(iob->buf)) {
		    if (sudo_ev_add(evbase, iob->revent, NULL, false) == -1)
			sudo_fatal(U_("unable to add event to queue"));
		}
	    }
	}
    }
}


I add assert there, but I don't know if it's right. What do you think about that?
I sent you patch as attachment.

Thanks.
Radovan S.

____________________________________________________________
sudo-workers mailing list <[email protected]>
For list information, options, or to unsubscribe, visit:
http://www.sudo.ws/mailman/listinfo/sudo-workers

diff -up sudo-1.8.12/src/exec_pty.c.fix sudo-1.8.12/src/exec_pty.c
--- sudo-1.8.12/src/exec_pty.c.fix	2015-07-07 16:59:50.816563858 +0200
+++ sudo-1.8.12/src/exec_pty.c	2015-07-07 17:01:32.494469777 +0200
@@ -22,6 +22,7 @@
 #include <sys/wait.h>
 #include <sys/ioctl.h>
 #include <stdio.h>
+#include <assert.h>
 #ifdef STDC_HEADERS
 # include <stdlib.h>
 # include <stddef.h>
@@ -563,6 +564,8 @@ io_callback(int fd, int what, void *v)
     int n;
     debug_decl(io_callback, SUDO_DEBUG_EXEC);
 
+    assert(!(ISSET(what, SUDO_EV_READ) && ISSET(what, SUDO_EV_WRITE)));
+
     if (ISSET(what, SUDO_EV_READ)) {
 	evbase = sudo_ev_get_base(iob->revent);
 	do {