infinite loop in xenstat_qmp.c

infinite loop in xenstat_qmp.c

[Reiser, Hans]

Hi,

I have seen several occasions with "dead" xentop processes consuming 100% CPU time, and tracked this down
to the following problem:

When the QEMU process the qmp_read function is communicating with terminates, qmp_read may enter an
infinite loop:  poll signals EOF (POLLIN and POLLHUP set), the subsequent read() call returns 0, and then the
function calls poll again, which still sees the EOF condition and will return again immediately with POLLIN and
POLLHUP set, repeating ad infinitum.

A simple fix is to terminate the loop when read returns 0 (under "normal" instances, poll will return with POLLIN
set only if there is data to read, so read will always read >0 bytes, except if the socket has been closed).

Cheers, Hans

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index 19b236e7b6..0c5748ba68 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -298,7 +298,7 @@ static int qmp_read(int qfd, unsigned char **qstats)
        pfd[0].events = POLLIN;
        while ((n = poll(pfd, 1, 10)) > 0) {
                if (pfd[0].revents & POLLIN) {
-                       if ((n = read(qfd, buf, sizeof(buf))) < 0) {
+                       if ((n = read(qfd, buf, sizeof(buf))) <= 0) {
                                free(*qstats);
                                return 0;
                        }

Re: infinite loop in xenstat_qmp.c

[Andrew Cooper]

On 09/11/2020 14:36, Reiser, Hans wrote:
> Hi,
>
> I have seen several occasions with "dead" xentop processes consuming 100% CPU time, and tracked this down
> to the following problem:
>
> When the QEMU process the qmp_read function is communicating with terminates, qmp_read may enter an
> infinite loop:  poll signals EOF (POLLIN and POLLHUP set), the subsequent read() call returns 0, and then the
> function calls poll again, which still sees the EOF condition and will return again immediately with POLLIN and
> POLLHUP set, repeating ad infinitum.
>
> A simple fix is to terminate the loop when read returns 0 (under "normal" instances, poll will return with POLLIN
> set only if there is data to read, so read will always read >0 bytes, except if the socket has been closed).
>
> Cheers, Hans

Hi - this appears to have slipped through the cracks.

Thanks for the bugfix, but we require code submissions to conform to the
DCO[1] and have a Signed-off-by line.

If you're happy to agree to that, I can fix up the patch and get it
sorted.  We've moved this library in the time since you submitted the
bugfix.

Thanks, and sorry for the delay.

~Andrew

[1]
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Signed-off-by

>
> diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> index 19b236e7b6..0c5748ba68 100644
> --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
> +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> @@ -298,7 +298,7 @@ static int qmp_read(int qfd, unsigned char **qstats)
>         pfd[0].events = POLLIN;
>         while ((n = poll(pfd, 1, 10)) > 0) {
>                 if (pfd[0].revents & POLLIN) {
> -                       if ((n = read(qfd, buf, sizeof(buf))) < 0) {
> +                       if ((n = read(qfd, buf, sizeof(buf))) <= 0) {
>                                 free(*qstats);
>                                 return 0;
>                         }
>
>

Re: infinite loop in xenstat_qmp.c

[Reiser, Hans]

Dear Andrew,
Thanks for picking this up. Sure, I agree with your proposal.
Cheers, Hans

> On 2021-01-22, at 14:25, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 09/11/2020 14:36, Reiser, Hans wrote:
>> Hi,
>> 
>> I have seen several occasions with "dead" xentop processes consuming 100% CPU time, and tracked this down
>> to the following problem:
>> 
>> When the QEMU process the qmp_read function is communicating with terminates, qmp_read may enter an
>> infinite loop:  poll signals EOF (POLLIN and POLLHUP set), the subsequent read() call returns 0, and then the
>> function calls poll again, which still sees the EOF condition and will return again immediately with POLLIN and
>> POLLHUP set, repeating ad infinitum.
>> 
>> A simple fix is to terminate the loop when read returns 0 (under "normal" instances, poll will return with POLLIN
>> set only if there is data to read, so read will always read >0 bytes, except if the socket has been closed).
>> 
>> Cheers, Hans
> 
> Hi - this appears to have slipped through the cracks.
> 
> Thanks for the bugfix, but we require code submissions to conform to the
> DCO[1] and have a Signed-off-by line.
> 
> If you're happy to agree to that, I can fix up the patch and get it
> sorted.  We've moved this library in the time since you submitted the
> bugfix.
> 
> Thanks, and sorry for the delay.
> 
> ~Andrew
> 
> [1]
> https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Signed-off-by
> 
>> 
>> diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
>> index 19b236e7b6..0c5748ba68 100644
>> --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
>> +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
>> @@ -298,7 +298,7 @@ static int qmp_read(int qfd, unsigned char **qstats)
>>        pfd[0].events = POLLIN;
>>        while ((n = poll(pfd, 1, 10)) > 0) {
>>                if (pfd[0].revents & POLLIN) {
>> -                       if ((n = read(qfd, buf, sizeof(buf))) < 0) {
>> +                       if ((n = read(qfd, buf, sizeof(buf))) <= 0) {
>>                                free(*qstats);
>>                                return 0;
>>                        }
>> 
>> 
>