* [PATCH] xenconsoled: use array index to keep track of pollfd
@ 2013-03-19 17:45 Wei Liu
2013-03-21 14:07 ` Marcus Granado
0 siblings, 1 reply; 3+ messages in thread
From: Wei Liu @ 2013-03-19 17:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Jackson, Wei Liu, Marcus Granado, Andrew Cooper
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);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] xenconsoled: use array index to keep track of pollfd
2013-03-19 17:45 [PATCH] xenconsoled: use array index to keep track of pollfd Wei Liu
@ 2013-03-21 14:07 ` Marcus Granado
2013-03-25 13:00 ` Ian Jackson
0 siblings, 1 reply; 3+ messages in thread
From: Marcus Granado @ 2013-03-21 14:07 UTC (permalink / raw)
To: Wei Liu; +Cc: Andrew Cooper, Ian Jackson, xen-devel
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] xenconsoled: use array index to keep track of pollfd
2013-03-21 14:07 ` Marcus Granado
@ 2013-03-25 13:00 ` Ian Jackson
0 siblings, 0 replies; 3+ messages in thread
From: Ian Jackson @ 2013-03-25 13:00 UTC (permalink / raw)
To: Marcus Granado; +Cc: Andrew Cooper, Wei Liu, xen-devel
Marcus Granado writes ("Re: [PATCH] xenconsoled: use array index to keep track of pollfd"):
> 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.
...
> 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>
Thanks to both of you. Committed.
Ian.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-03-25 13:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-19 17:45 [PATCH] xenconsoled: use array index to keep track of pollfd Wei Liu
2013-03-21 14:07 ` Marcus Granado
2013-03-25 13:00 ` Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.