diff mbox

tcg: Reload local variables after return from longjmp

Message ID 4E0EE837.1000605@web.de
State New
Headers show

Commit Message

Jan Kiszka July 2, 2011, 9:43 a.m. UTC 
On 2011-07-02 11:08, Blue Swirl wrote:
> On Sat, Jul 2, 2011 at 10:50 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Recent compilers look deep into cpu_exec, find longjmp as a noreturn
>> function and decide to smash some stack variables as they won't be used
>> again. This may lead to env becoming invalid after return from setjmp,
>> causing crashes. Fix it by reloading env from cpu_single_env in that
>> case.
> 
> Nice. Could you try if gcc flag -Wclobbered catches something using
> your compiler without your patch:
> 
> commit f826f0d0f5cf5dd18a0d34159c1a3bc8f2e6ddf4
> Author: Blue Swirl <blauwirbel@gmail.com>
> Date:   Sun Sep 26 11:58:38 2010 +0000
> 
>     Add gcc warning -Wclobbered
> 
>     Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> 
> diff --git a/configure b/configure
> index 88159ac..2417205 100755
> --- a/configure
> +++ b/configure
> @@ -1038,7 +1038,7 @@ fi
>  gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
>  gcc_flags="-Wformat-security -Wformat-y2k -Winit-self
> -Wignored-qualifiers $gcc_flags"
>  gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
> -gcc_flags="-fstack-protector-all -Wendif-labels $gcc_flags"
> +gcc_flags="-fstack-protector-all -Wendif-labels -Wclobbered $gcc_flags"
>  cat > $TMPC << EOF
>  int main(void) { return 0; }
>  EOF

Neither native gcc 4.5.0 nor mingw's 4.6.0 detect this issue. However,
4.6 found this - and 3 false positives:



But the right way to deal with it is to return error codes and get rid
of this setjmp/longjmp mess for get_expr.

Jan

Comments

Paolo Bonzini July 3, 2011, 2:09 p.m. UTC | #1 
On 07/02/2011 11:43 AM, Jan Kiszka wrote:
>   static const char *pch;
> +static char *saved_key;
>   static jmp_buf expr_env;
>
>   #define MD_TLONG 0
> @@ -4254,8 +4255,11 @@ static const mon_cmd_t *monitor_parse_command(Monitor *mon,
>                       }
>                       typestr++;
>                   }
> -                if (get_expr(mon,&val,&p))
> +                saved_key = key;
> +                if (get_expr(mon,&val,&p)) {
> +                    key = saved_key;
>                       goto fail;
> +                }

Please make saved_key a volatile local instead.

Paolo
diff mbox

Patch

diff --git a/monitor.c b/monitor.c
index 67ceb46..d7edbbb 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3253,6 +3253,7 @@  static const mon_cmd_t qmp_query_cmds[] = {
 /*******************************************************************/
 
 static const char *pch;
+static char *saved_key;
 static jmp_buf expr_env;
 
 #define MD_TLONG 0
@@ -4254,8 +4255,11 @@  static const mon_cmd_t *monitor_parse_command(Monitor *mon,
                     }
                     typestr++;
                 }
-                if (get_expr(mon, &val, &p))
+                saved_key = key;
+                if (get_expr(mon, &val, &p)) {
+                    key = saved_key;
                     goto fail;
+                }
                 /* Check if 'i' is greater than 32-bit */
                 if ((c == 'i') && ((val >> 32) & 0xffffffff)) {
                     monitor_printf(mon, "\'%s\' has failed: ", cmdname);