Re: [PATCH] xenconsoled: use array index to keep track of pollfd

[Marcus Granado <marcus.granado@citrix.com>]

On 19/03/13 17:45, Wei Liu wrote:
> If we use pointers to reference elements inside array, it is possible that we
> get wild pointer after realloc(3) copies array and returns a new pointer.
>
> Keep track of element indexes inside the array can solve this problem.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Marcus Granado <marcus.granado@citrix.com>
> ---
>   tools/console/daemon/io.c |   66 ++++++++++++++++++++++++---------------------
>   1 file changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> index 50f91b5..250550a 100644
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -87,7 +87,7 @@ struct buffer {
>   struct domain {
>   	int domid;
>   	int master_fd;
> -	struct pollfd *master_pollfd;
> +	int master_pollfd_idx;
>   	int slave_fd;
>   	int log_fd;
>   	bool is_dead;
> @@ -99,7 +99,7 @@ struct domain {
>   	evtchn_port_or_error_t local_port;
>   	evtchn_port_or_error_t remote_port;
>   	xc_evtchn *xce_handle;
> -	struct pollfd *xce_pollfd;
> +	int xce_pollfd_idx;
>   	struct xencons_interface *interface;
>   	int event_count;
>   	long long next_period;
> @@ -669,8 +669,10 @@ static struct domain *create_domain(int domid)
>   	strcat(dom->conspath, "/console");
>
>   	dom->master_fd = -1;
> +	dom->master_pollfd_idx = -1;
>   	dom->slave_fd = -1;
>   	dom->log_fd = -1;
> +	dom->xce_pollfd_idx = -1;
>
>   	dom->next_period = ((long long)ts.tv_sec * 1000) + (ts.tv_nsec / 1000000) + RATE_LIMIT_PERIOD;
>
> @@ -944,9 +946,10 @@ static void handle_log_reload(void)
>   	}
>   }
>
> -static struct pollfd *set_fds(int fd, short events)
> +/* Returns index inside fds array if succees, -1 if fail */
> +static int set_fds(int fd, short events)
>   {
> -	struct pollfd *ret;
> +	int ret;
>   	if (current_array_size < nr_fds + 1) {
>   		struct pollfd  *new_fds = NULL;
>   		unsigned long newsize;
> @@ -968,27 +971,28 @@ static struct pollfd *set_fds(int fd, short events)
>
>   	fds[nr_fds].fd = fd;
>   	fds[nr_fds].events = events;
> -	ret = &fds[nr_fds];
> +	ret = nr_fds;
>   	nr_fds++;
>
>   	return ret;
>   fail:
>   	dolog(LOG_ERR, "realloc failed, ignoring fd %d\n", fd);
> -	return NULL;
> +	return -1;
>   }
>
>   static void reset_fds(void)
>   {
>   	nr_fds = 0;
> -	memset(fds, 0, sizeof(struct pollfd) * current_array_size);
> +	if (fds)
> +		memset(fds, 0, sizeof(struct pollfd) * current_array_size);
>   }
>
>   void handle_io(void)
>   {
>   	int ret;
>   	evtchn_port_or_error_t log_hv_evtchn = -1;
> -	struct pollfd *xce_pollfd = NULL;
> -	struct pollfd *xs_pollfd = NULL;
> +	int xce_pollfd_idx = -1;
> +	int xs_pollfd_idx = -1;
>   	xc_evtchn *xce_handle = NULL;
>
>   	if (log_hv) {
> @@ -1025,11 +1029,11 @@ void handle_io(void)
>
>   		reset_fds();
>
> -		xs_pollfd = set_fds(xs_fileno(xs), POLLIN|POLLPRI);
> +		xs_pollfd_idx = set_fds(xs_fileno(xs), POLLIN|POLLPRI);
>
>   		if (log_hv)
> -			xce_pollfd = set_fds(xc_evtchn_fd(xce_handle),
> -					     POLLIN|POLLPRI);
> +			xce_pollfd_idx = set_fds(xc_evtchn_fd(xce_handle),
> +						 POLLIN|POLLPRI);
>
>   		if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0)
>   			return;
> @@ -1064,8 +1068,8 @@ void handle_io(void)
>   				    !d->buffer.max_capacity ||
>   				    d->buffer.size < d->buffer.max_capacity) {
>   					int evtchn_fd = xc_evtchn_fd(d->xce_handle);
> -					d->xce_pollfd = set_fds(evtchn_fd,
> -								POLLIN|POLLPRI);
> +					d->xce_pollfd_idx = set_fds(evtchn_fd,
> +								    POLLIN|POLLPRI);
>   				}
>   			}
>
> @@ -1078,7 +1082,7 @@ void handle_io(void)
>   					events |= POLLOUT;
>
>   				if (events)
> -					d->master_pollfd =
> +					d->master_pollfd_idx =
>   						set_fds(d->master_fd,
>   							events|POLLPRI);
>   			}
> @@ -1110,61 +1114,61 @@ void handle_io(void)
>   			break;
>   		}
>
> -		if (log_hv && xce_pollfd) {
> -			if (xce_pollfd->revents & ~(POLLIN|POLLOUT|POLLPRI)) {
> +		if (log_hv && xce_pollfd_idx != -1) {
> +			if (fds[xce_pollfd_idx].revents & ~(POLLIN|POLLOUT|POLLPRI)) {
>   				dolog(LOG_ERR,
>   				      "Failure in poll xce_handle: %d (%s)",
>   				      errno, strerror(errno));
>   				break;
> -			} else if (xce_pollfd->revents & POLLIN)
> +			} else if (fds[xce_pollfd_idx].revents & POLLIN)
>   				handle_hv_logs(xce_handle);
>
> -			xce_pollfd = NULL;
> +			xce_pollfd_idx = -1;
>   		}
>
>   		if (ret <= 0)
>   			continue;
>
> -		if (xs_pollfd) {
> -			if (xs_pollfd->revents & ~(POLLIN|POLLOUT|POLLPRI)) {
> +		if (xs_pollfd_idx != -1) {
> +			if (fds[xs_pollfd_idx].revents & ~(POLLIN|POLLOUT|POLLPRI)) {
>   				dolog(LOG_ERR,
>   				      "Failure in poll xs_handle: %d (%s)",
>   				      errno, strerror(errno));
>   				break;
> -			} else if (xs_pollfd->revents & POLLIN)
> +			} else if (fds[xs_pollfd_idx].revents & POLLIN)
>   				handle_xs();
>
> -			xs_pollfd = NULL;
> +			xs_pollfd_idx = -1;
>   		}
>
>   		for (d = dom_head; d; d = n) {
>   			n = d->next;
>   			if (d->event_count < RATE_LIMIT_ALLOWANCE) {
>   				if (d->xce_handle != NULL &&
> -				    d->xce_pollfd &&
> -				    !(d->xce_pollfd->revents &
> +				    d->xce_pollfd_idx != -1 &&
> +				    !(fds[d->xce_pollfd_idx].revents &
>   				      ~(POLLIN|POLLOUT|POLLPRI)) &&
> -				      (d->xce_pollfd->revents &
> +				      (fds[d->xce_pollfd_idx].revents &
>   				       POLLIN))
>   				    handle_ring_read(d);
>   			}
>
> -			if (d->master_fd != -1 && d->master_pollfd) {
> -				if (d->master_pollfd->revents &
> +			if (d->master_fd != -1 && d->master_pollfd_idx != -1) {
> +				if (fds[d->master_pollfd_idx].revents &
>   				    ~(POLLIN|POLLOUT|POLLPRI))
>   					domain_handle_broken_tty(d,
>   						   domain_is_valid(d->domid));
>   				else {
> -					if (d->master_pollfd->revents &
> +					if (fds[d->master_pollfd_idx].revents &
>   					    POLLIN)
>   						handle_tty_read(d);
> -					if (d->master_pollfd->revents &
> +					if (fds[d->master_pollfd_idx].revents &
>   					    POLLOUT)
>   						handle_tty_write(d);
>   				}
>   			}
>
> -			d->xce_pollfd = d->master_pollfd = NULL;
> +			d->xce_pollfd_idx = d->master_pollfd_idx = -1;
>
>   			if (d->last_seen != enum_pass)
>   				shutdown_domain(d);
>

I was able to start 700 vms in a host using the patch above.

Reviewed-by: Marcus Granado <marcus.granado@citrix.com>
Tested-by: Marcus Granado <marcus.granado@citrix.com>