[PATCH 1/7] Samba-VirusFilter: add write_data_timeout ... version 3

[Jeremy Allison]

On Fri, Oct 21, 2016 at 11:57:11AM -0600, Trever L. Adams wrote:
> Samba-VirusFilter: add write_data_timeout and write_data_iov_timeout.
> 
> This patch adds those functions to sys_rw_data.[ch].
> 
> Signed-off-by: Trever L. Adams <trever.adams at gmail.com>

I'm trying to live with write_data_iov_timeout() but I can't. It
just bugs me :-).

int cont_eintr

needs removing. All uses in the VirusFilter code pass
'1' here, so it really shouldn't be there.

So given that, this function wants to continue on EINTR,
but it doesn't do that. It has:

 +     case -1:
 +             if (errno == EINTR && cont_eintr) {
 +                     errno = 0;
 +             }

and then goes into the sys_writev() call. But that
wasn't a 'socket is ready' message, it was a EINTR
-1 return - so this could block.

In the later code once it's sent the first amount of
data it does:

 +     while (sent < to_send) {
 +             bool ok;
 +
 +             ok = iov_advance(&iov, &iovcnt, thistime);
 +             if (!ok) {
 +                     errno = EIO;
 +                     return -1;
 +             }
 +
 +             switch (poll(&pollfd, 1, msec_timeout)) {
 +             case -1:
 +                     if (errno == EINTR) {
 +                             errno = 0;
 +                             continue;
 +                     }

This ignores EINTR, but doesn't decrement the timeout
given the time that's already passed.

Both these problems mean that 'msec_timeout' is not a time
limit, as judiciously received signals can cause essentially
infinite extension on the write timeout.

This looks clumsy to me. All you're trying to do is
send a text message down a socket to an external process,
with a fixed timeout on the send, and this doesn't quite
do the trick reliably.

Worse, it's reinventing (with bugs) functionality we
already have in the tstream_context abstraction.

I think this code should connect using tstream_unix_socketpair(),
create a temp event context to loop on, set a timer event to fire
after msec_timeout, and then use tstream_writev_send()/tstream_writev_recv()
to push the data down the socket.

That way will correctly handle timeouts and errors in
communication with the external anti-virus engine.

If you can't get to this I can try and do this for you, but it'll require
changes in the other anti-virus code as you won't just be
storing an int io_h->socket anymore, but a struct tstream_context *.

Jeremy.

> From 5c354653b0146feece4718c84698f930ff6b6cdf Mon Sep 17 00:00:00 2001
> From: "Trever L. Adams" <trever.adams at gmail.com>
> Date: Tue, 18 Oct 2016 13:32:53 -0600
> Subject: [PATCH 1/7] Samba-VirusFilter: add write_data_timeout and
>  write_data_iov_timeout.
> 
> This patch adds those functions to sys_rw_data.[ch].
> 
> Signed-off-by: Trever L. Adams <trever.adams at gmail.com>
> ---
>  lib/util/sys_rw_data.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/util/sys_rw_data.h |   5 +++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/lib/util/sys_rw_data.c b/lib/util/sys_rw_data.c
> index de71716..bc8aa00 100644
> --- a/lib/util/sys_rw_data.c
> +++ b/lib/util/sys_rw_data.c
> @@ -20,6 +20,7 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <poll.h>
>  #include "replace.h"
>  #include "system/filesys.h"
>  #include "lib/util/sys_rw_data.h"
> @@ -115,3 +116,107 @@ ssize_t read_data(int fd, void *buffer, size_t n)
>  
>  	return nread;
>  }
> +
> +/****************************************************************************
> + Write all data from an iov array, with msec timeout (per write)
> + NB. This can be called with a non-socket fd, don't add dependencies
> + on socket calls.
> +****************************************************************************/
> +
> +ssize_t write_data_iov_timeout(int fd, const struct iovec *orig_iov,
> +	int iovcnt, int msec_timeout, int cont_eintr)
> +{
> +	ssize_t to_send;
> +	ssize_t thistime;
> +	size_t sent;
> +	struct iovec iov_copy[iovcnt];
> +	struct iovec *iov;
> +	struct pollfd pollfd;
> +
> +	pollfd.fd = fd;
> +	pollfd.events = POLLOUT;
> +
> +	to_send = iov_buflen(orig_iov, iovcnt);
> +	if (to_send == -1) {
> +		errno = EINVAL;
> +		return -1;
> +	}
> +
> +	switch (poll(&pollfd, 1, msec_timeout)) {
> +	case -1:
> +		if (errno == EINTR && cont_eintr) {
> +			errno = 0;
> +		}
> +		else return -1;
> +	case 0:
> +		errno = ETIMEDOUT;
> +		return -1;
> +	}
> +
> +	thistime = sys_writev(fd, orig_iov, iovcnt);
> +	if ((thistime <= 0) || (thistime == to_send)) {
> +		if (thistime <= 0 && errno == EINTR && cont_eintr) {
> +			errno = 0;
> +		}
> +		else return thistime;
> +	}
> +	sent = thistime;
> +
> +	/*
> +	 * We could not send everything in one call. Make a copy of iov that
> +	 * we can mess with.
> +	 */
> +
> +	memcpy(iov_copy, orig_iov, sizeof(struct iovec) * iovcnt);
> +	iov = iov_copy;
> +
> +	while (sent < to_send) {
> +		bool ok;
> +
> +		ok = iov_advance(&iov, &iovcnt, thistime);
> +		if (!ok) {
> +			errno = EIO;
> +			return -1;
> +		}
> +
> +		switch (poll(&pollfd, 1, msec_timeout)) {
> +		case -1:
> +			if (errno == EINTR) {
> +				errno = 0;
> +				continue;
> +			}
> +			return -1;
> +		case 0:
> +			errno = ETIMEDOUT;
> +			return -1;
> +		}
> +
> +		thistime = sys_writev(fd, iov, iovcnt);
> +		if (thistime <= 0) {
> +			if (errno == EINTR && cont_eintr) {
> +				errno = 0;
> +				continue;
> +			}
> +			break;
> +		}
> +		sent += thistime;
> +	}
> +
> +	return sent;
> +}
> +
> +/****************************************************************************
> + Write data to a fd, with msec timeout (per write)
> + NB. This can be called with a non-socket fd, don't add dependencies
> + on socket calls.
> +****************************************************************************/
> +
> +ssize_t write_data_timeout(int fd, const void *buffer, size_t n, int timeout,
> +	int cont_eintr)
> +{
> +	struct iovec iov;
> +
> +	iov.iov_base = discard_const_p(void, buffer);
> +	iov.iov_len = n;
> +	return write_data_iov_timeout(fd, &iov, 1, timeout, cont_eintr);
> +}
> diff --git a/lib/util/sys_rw_data.h b/lib/util/sys_rw_data.h
> index bda3795..e0a6fd2 100644
> --- a/lib/util/sys_rw_data.h
> +++ b/lib/util/sys_rw_data.h
> @@ -31,4 +31,9 @@ ssize_t write_data_iov(int fd, const struct iovec *iov, int iovcnt);
>  ssize_t write_data(int fd, const void *buffer, size_t n);
>  ssize_t read_data(int fd, void *buffer, size_t n);
>  
> +ssize_t write_data_iov_timeout(int fd, const struct iovec *orig_iov,
> +	int iovcnt, int msec_timeout, int cont_eintr);
> +ssize_t write_data_timeout(int fd, const void *buffer, size_t n, int timeout,
> +	int cont_eintr);
> +
>  #endif
> -- 
> 2.9.3
>