[PATCH] ui/curses: Fix infinite loop on windows

[PATCH] ui/curses: Fix infinite loop on windows

[William Hu via]

From a42046272f0544dd18ed58661e53ea17d1584c2c Mon Sep 17 00:00:00 2001
From: William Hu <purplearmadillo77@proton.me>
Date: Wed, 2 Apr 2025 12:00:00 -0400
Subject: [PATCH] ui/curses: Fix infinite loop on windows

Replace -1 comparisons for wint_t with WEOF to fix infinite loop caused by a
65535 == -1 comparison.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2905
Signed-off-by: William Hu <purplearmadillo77@proton.me>
---
 ui/curses.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/ui/curses.c b/ui/curses.c
index a39aee8762..3f5c5adf78 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -265,7 +265,12 @@ static int curses2foo(const int _curses2foo[], const int _curseskey2foo[],
 
 static void curses_refresh(DisplayChangeListener *dcl)
 {
-    int chr, keysym, keycode, keycode_alt;
+    /*
+     * DO NOT MAKE chr AN INT:
+     * Causes silent conversion errors on Windows where wint_t is unsigned short.
+     */
+    wint_t chr = 0;
+    int keysym, keycode, keycode_alt;
     enum maybe_keycode maybe_keycode = CURSES_KEYCODE;
 
     curses_winch_check();
@@ -284,8 +289,9 @@ static void curses_refresh(DisplayChangeListener *dcl)
         /* while there are any pending key strokes to process */
         chr = console_getch(&maybe_keycode);
 
-        if (chr == -1)
+        if (chr == WEOF) {
             break;
+        }
 
 #ifdef KEY_RESIZE
         /* this shouldn't occur when we use a custom SIGWINCH handler */
-- 
2.47.0

Re: [PATCH] ui/curses: Fix infinite loop on windows

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

On Thu, Apr 03, 2025 at 01:07:56AM +0000, William Hu via wrote:
> >From a42046272f0544dd18ed58661e53ea17d1584c2c Mon Sep 17 00:00:00 2001
> From: William Hu <purplearmadillo77@proton.me>
> Date: Wed, 2 Apr 2025 12:00:00 -0400
> Subject: [PATCH] ui/curses: Fix infinite loop on windows
> 
> Replace -1 comparisons for wint_t with WEOF to fix infinite loop caused by a
> 65535 == -1 comparison.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2905
> Signed-off-by: William Hu <purplearmadillo77@proton.me>
> ---
>  ui/curses.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

I have CCed Gerd Hoffmann (git-shortlog(1) shows he is the most frequent
committer to this source file) and Marc-André Lureau (ui/ maintainer
according to the ./MAINTAINERS file) so they can also review your patch.

> 
> diff --git a/ui/curses.c b/ui/curses.c
> index a39aee8762..3f5c5adf78 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -265,7 +265,12 @@ static int curses2foo(const int _curses2foo[], const int _curseskey2foo[],
>  
>  static void curses_refresh(DisplayChangeListener *dcl)
>  {
> -    int chr, keysym, keycode, keycode_alt;
> +    /*
> +     * DO NOT MAKE chr AN INT:
> +     * Causes silent conversion errors on Windows where wint_t is unsigned short.
> +     */
> +    wint_t chr = 0;
> +    int keysym, keycode, keycode_alt;
>      enum maybe_keycode maybe_keycode = CURSES_KEYCODE;
>  
>      curses_winch_check();
> @@ -284,8 +289,9 @@ static void curses_refresh(DisplayChangeListener *dcl)
>          /* while there are any pending key strokes to process */
>          chr = console_getch(&maybe_keycode);
>  
> -        if (chr == -1)
> +        if (chr == WEOF) {
>              break;
> +        }

Further below there appears to be another instance of the same bug:

  /* alt or esc key */
  if (keycode == 1) {
      enum maybe_keycode next_maybe_keycode = CURSES_KEYCODE;
      int nextchr = console_getch(&next_maybe_keycode);

      if (nextchr != -1) {
          ^^^^^^^^^^^^^

>  
>  #ifdef KEY_RESIZE
>          /* this shouldn't occur when we use a custom SIGWINCH handler */
> -- 
> 2.47.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ui/curses: Fix infinite loop on windows

[Philippe Mathieu-Daudé]

On 3/4/25 03:07, William Hu via wrote:
>  From a42046272f0544dd18ed58661e53ea17d1584c2c Mon Sep 17 00:00:00 2001
> From: William Hu <purplearmadillo77@proton.me>
> Date: Wed, 2 Apr 2025 12:00:00 -0400
> Subject: [PATCH] ui/curses: Fix infinite loop on windows
> 
> Replace -1 comparisons for wint_t with WEOF to fix infinite loop caused by a
> 65535 == -1 comparison.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2905
> Signed-off-by: William Hu <purplearmadillo77@proton.me>
> ---
>   ui/curses.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/ui/curses.c b/ui/curses.c
> index a39aee8762..3f5c5adf78 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -265,7 +265,12 @@ static int curses2foo(const int _curses2foo[], const int _curseskey2foo[],
>   
>   static void curses_refresh(DisplayChangeListener *dcl)
>   {
> -    int chr, keysym, keycode, keycode_alt;
> +    /*
> +     * DO NOT MAKE chr AN INT:
> +     * Causes silent conversion errors on Windows where wint_t is unsigned short.
> +     */
> +    wint_t chr = 0;
> +    int keysym, keycode, keycode_alt;
>       enum maybe_keycode maybe_keycode = CURSES_KEYCODE;
>   
>       curses_winch_check();
> @@ -284,8 +289,9 @@ static void curses_refresh(DisplayChangeListener *dcl)
>           /* while there are any pending key strokes to process */
>           chr = console_getch(&maybe_keycode);
>   
> -        if (chr == -1)
> +        if (chr == WEOF) {
>               break;
> +        }

Correct but incomplete, also missing the same check few lines below:

-- >8 --
diff --git a/ui/curses.c b/ui/curses.c
index a39aee87623..9c33de331cd 100644
--- a/ui/curses.c
+++ b/ui/curses.c
@@ -304,9 +304,9 @@ static void curses_refresh(DisplayChangeListener *dcl)
          /* alt or esc key */
          if (keycode == 1) {
              enum maybe_keycode next_maybe_keycode = CURSES_KEYCODE;
-            int nextchr = console_getch(&next_maybe_keycode);
+            wint_t nextchr = console_getch(&next_maybe_keycode);

-            if (nextchr != -1) {
+            if (nextchr != WEOF) {
                  chr = nextchr;
                  maybe_keycode = next_maybe_keycode;
                  keycode_alt = ALT;
---

With that:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH] ui/curses: Fix infinite loop on windows

[Philippe Mathieu-Daudé]

On 8/4/25 16:08, Stefan Hajnoczi wrote:
> On Thu, Apr 03, 2025 at 01:07:56AM +0000, William Hu via wrote:
>> >From a42046272f0544dd18ed58661e53ea17d1584c2c Mon Sep 17 00:00:00 2001
>> From: William Hu <purplearmadillo77@proton.me>
>> Date: Wed, 2 Apr 2025 12:00:00 -0400
>> Subject: [PATCH] ui/curses: Fix infinite loop on windows
>>
>> Replace -1 comparisons for wint_t with WEOF to fix infinite loop caused by a
>> 65535 == -1 comparison.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2905
>> Signed-off-by: William Hu <purplearmadillo77@proton.me>
>> ---
>>   ui/curses.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> I have CCed Gerd Hoffmann (git-shortlog(1) shows he is the most frequent
> committer to this source file) and Marc-André Lureau (ui/ maintainer
> according to the ./MAINTAINERS file) so they can also review your patch.
> 
>>
>> diff --git a/ui/curses.c b/ui/curses.c
>> index a39aee8762..3f5c5adf78 100644
>> --- a/ui/curses.c
>> +++ b/ui/curses.c
>> @@ -265,7 +265,12 @@ static int curses2foo(const int _curses2foo[], const int _curseskey2foo[],
>>   
>>   static void curses_refresh(DisplayChangeListener *dcl)
>>   {
>> -    int chr, keysym, keycode, keycode_alt;
>> +    /*
>> +     * DO NOT MAKE chr AN INT:
>> +     * Causes silent conversion errors on Windows where wint_t is unsigned short.
>> +     */
>> +    wint_t chr = 0;
>> +    int keysym, keycode, keycode_alt;
>>       enum maybe_keycode maybe_keycode = CURSES_KEYCODE;
>>   
>>       curses_winch_check();
>> @@ -284,8 +289,9 @@ static void curses_refresh(DisplayChangeListener *dcl)
>>           /* while there are any pending key strokes to process */
>>           chr = console_getch(&maybe_keycode);
>>   
>> -        if (chr == -1)
>> +        if (chr == WEOF) {
>>               break;
>> +        }
> 
> Further below there appears to be another instance of the same bug:
> 
>    /* alt or esc key */
>    if (keycode == 1) {
>        enum maybe_keycode next_maybe_keycode = CURSES_KEYCODE;
>        int nextchr = console_getch(&next_maybe_keycode);
> 
>        if (nextchr != -1) {
>            ^^^^^^^^^^^^^

Indeed.

The changes comes from commit 459a707eccc ("curses: support wide input")
from 2019. This isn't a blocker for the next release IMHO.

Re: [PATCH-for-10.1] ui/curses: Fix infinite loop on windows

[Philippe Mathieu-Daudé]

Ping?

On 8/4/25 21:01, Philippe Mathieu-Daudé wrote:
> On 3/4/25 03:07, William Hu via wrote:
>>  From a42046272f0544dd18ed58661e53ea17d1584c2c Mon Sep 17 00:00:00 2001
>> From: William Hu <purplearmadillo77@proton.me>
>> Date: Wed, 2 Apr 2025 12:00:00 -0400
>> Subject: [PATCH] ui/curses: Fix infinite loop on windows
>>
>> Replace -1 comparisons for wint_t with WEOF to fix infinite loop 
>> caused by a
>> 65535 == -1 comparison.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2905
>> Signed-off-by: William Hu <purplearmadillo77@proton.me>
>> ---
>>   ui/curses.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/ui/curses.c b/ui/curses.c
>> index a39aee8762..3f5c5adf78 100644
>> --- a/ui/curses.c
>> +++ b/ui/curses.c
>> @@ -265,7 +265,12 @@ static int curses2foo(const int _curses2foo[], 
>> const int _curseskey2foo[],
>>   static void curses_refresh(DisplayChangeListener *dcl)
>>   {
>> -    int chr, keysym, keycode, keycode_alt;
>> +    /*
>> +     * DO NOT MAKE chr AN INT:
>> +     * Causes silent conversion errors on Windows where wint_t is 
>> unsigned short.
>> +     */
>> +    wint_t chr = 0;
>> +    int keysym, keycode, keycode_alt;
>>       enum maybe_keycode maybe_keycode = CURSES_KEYCODE;
>>       curses_winch_check();
>> @@ -284,8 +289,9 @@ static void curses_refresh(DisplayChangeListener 
>> *dcl)
>>           /* while there are any pending key strokes to process */
>>           chr = console_getch(&maybe_keycode);
>> -        if (chr == -1)
>> +        if (chr == WEOF) {
>>               break;
>> +        }
> 
> Correct but incomplete, also missing the same check few lines below:
> 
> -- >8 --
> diff --git a/ui/curses.c b/ui/curses.c
> index a39aee87623..9c33de331cd 100644
> --- a/ui/curses.c
> +++ b/ui/curses.c
> @@ -304,9 +304,9 @@ static void curses_refresh(DisplayChangeListener *dcl)
>           /* alt or esc key */
>           if (keycode == 1) {
>               enum maybe_keycode next_maybe_keycode = CURSES_KEYCODE;
> -            int nextchr = console_getch(&next_maybe_keycode);
> +            wint_t nextchr = console_getch(&next_maybe_keycode);
> 
> -            if (nextchr != -1) {
> +            if (nextchr != WEOF) {
>                   chr = nextchr;
>                   maybe_keycode = next_maybe_keycode;
>                   keycode_alt = ALT;
> ---
> 
> With that:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>