[Qemu-devel] [PATCH] PowerPC: Avoid segfault in cpu_dump_state

[Qemu-devel] [PATCH] PowerPC: Avoid segfault in cpu_dump_state

[Fabien Chouteau]

Quit if no log file is defined.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 target-ppc/translate.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index cf59765..f17bd91 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -9319,6 +9319,10 @@ void cpu_dump_state (CPUPPCState *env, FILE *f, fprintf_function cpu_fprintf,
 
     int i;
 
+    if (f == NULL) {
+        return;
+    }
+
     cpu_synchronize_state(env);
 
     cpu_fprintf(f, "NIP " TARGET_FMT_lx "   LR " TARGET_FMT_lx " CTR "
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] PowerPC: Avoid segfault in cpu_dump_state

[Peter Maydell]

On 14 May 2012 15:46, Fabien Chouteau <chouteau@adacore.com> wrote:
> Quit if no log file is defined.
>
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  target-ppc/translate.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index cf59765..f17bd91 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -9319,6 +9319,10 @@ void cpu_dump_state (CPUPPCState *env, FILE *f, fprintf_function cpu_fprintf,
>
>     int i;
>
> +    if (f == NULL) {
> +        return;
> +    }
> +
>     cpu_synchronize_state(env);
>
>     cpu_fprintf(f, "NIP " TARGET_FMT_lx "   LR " TARGET_FMT_lx " CTR "

target-ppc isn't the only one that doesn't check for a NULL f:
perhaps it would be better to say "you can't call this with a
NULL FILE*" and fix whatever is calling it in that way?

-- PMM

[Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Fabien Chouteau]

Do not call cpu_dump_state if logfile is NULL.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
 qemu-log.h |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/qemu-log.h b/qemu-log.h
index fccfb110..2cd5ffa 100644
--- a/qemu-log.h
+++ b/qemu-log.h
@@ -51,7 +51,12 @@ extern int loglevel;
 /* Special cases: */
 
 /* cpu_dump_state() logging functions: */
-#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
+#define log_cpu_state(env, f)                          \
+do {                                                   \
+    if (logfile != NULL) {                             \
+        cpu_dump_state((env), logfile, fprintf, (f));  \
+    }                                                  \
+ } while (0)
 #define log_cpu_state_mask(b, env, f) do {           \
       if (loglevel & (b)) log_cpu_state((env), (f)); \
   } while (0)
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Andreas Färber]

Am 15.05.2012 11:39, schrieb Fabien Chouteau:
> Do not call cpu_dump_state if logfile is NULL.

And where is log_cpu_state() being called from? Its caller is passing
NULL already then.

Andreas

> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
> ---
>  qemu-log.h |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-log.h b/qemu-log.h
> index fccfb110..2cd5ffa 100644
> --- a/qemu-log.h
> +++ b/qemu-log.h
> @@ -51,7 +51,12 @@ extern int loglevel;
>  /* Special cases: */
>  
>  /* cpu_dump_state() logging functions: */
> -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
> +#define log_cpu_state(env, f)                          \
> +do {                                                   \
> +    if (logfile != NULL) {                             \
> +        cpu_dump_state((env), logfile, fprintf, (f));  \
> +    }                                                  \
> + } while (0)
>  #define log_cpu_state_mask(b, env, f) do {           \
>        if (loglevel & (b)) log_cpu_state((env), (f)); \
>    } while (0)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Fabien Chouteau]

On 05/15/2012 03:31 PM, Andreas Färber wrote:
> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>> Do not call cpu_dump_state if logfile is NULL.
> 
> And where is log_cpu_state() being called from? Its caller is passing
> NULL already then.
> 

No, logfile is a global variable. log_cpu_state() takes only CPUState
and flags parameters.


>>
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>> ---
>>  qemu-log.h |    7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/qemu-log.h b/qemu-log.h
>> index fccfb110..2cd5ffa 100644
>> --- a/qemu-log.h
>> +++ b/qemu-log.h
>> @@ -51,7 +51,12 @@ extern int loglevel;
>>  /* Special cases: */
>>  
>>  /* cpu_dump_state() logging functions: */
>> -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
>> +#define log_cpu_state(env, f)                          \
>> +do {                                                   \
>> +    if (logfile != NULL) {                             \
>> +        cpu_dump_state((env), logfile, fprintf, (f));  \
>> +    }                                                  \
>> + } while (0)
>>  #define log_cpu_state_mask(b, env, f) do {           \
>>        if (loglevel & (b)) log_cpu_state((env), (f)); \
>>    } while (0)
> 

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Peter Maydell]

On 15 May 2012 17:08, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>> Do not call cpu_dump_state if logfile is NULL.
>>
>> And where is log_cpu_state() being called from? Its caller is passing
>> NULL already then.

> No, logfile is a global variable. log_cpu_state() takes only CPUState
> and flags parameters.

The question is which of the following two options we want:
(1) callers should be guarding the calls to log_cpu_state() with
checks for qemu_log_enabled() or qemu_loglevel_mask()
(2) log_cpu_state() does its own check for whether logging is enabled
in the same way that qemu_log() and qemu_log_vprintf() do

At the moment most callers of log_cpu_state() do their own checks
as per (1), but you could make an argument that we should switch
to (2) instead.

-- PMM

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Fabien Chouteau]

On 05/15/2012 06:20 PM, Peter Maydell wrote:
> On 15 May 2012 17:08, Fabien Chouteau <chouteau@adacore.com> wrote:
>> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>>> Do not call cpu_dump_state if logfile is NULL.
>>>
>>> And where is log_cpu_state() being called from? Its caller is passing
>>> NULL already then.
> 
>> No, logfile is a global variable. log_cpu_state() takes only CPUState
>> and flags parameters.
> 
> The question is which of the following two options we want:
> (1) callers should be guarding the calls to log_cpu_state() with
> checks for qemu_log_enabled() or qemu_loglevel_mask()
> (2) log_cpu_state() does its own check for whether logging is enabled
> in the same way that qemu_log() and qemu_log_vprintf() do
> 
> At the moment most callers of log_cpu_state() do their own checks
> as per (1), but you could make an argument that we should switch
> to (2) instead.
> 

I think (2) is better, we do the check in one place and that's it. All
call to log_cpu_state are safe. And as you said, it's already the way
qemu_log and qemu_log_vprintf work.

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Peter Maydell]

On 15 May 2012 17:45, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 05/15/2012 06:20 PM, Peter Maydell wrote:
>> The question is which of the following two options we want:
>> (1) callers should be guarding the calls to log_cpu_state() with
>> checks for qemu_log_enabled() or qemu_loglevel_mask()
>> (2) log_cpu_state() does its own check for whether logging is enabled
>> in the same way that qemu_log() and qemu_log_vprintf() do
>>
>> At the moment most callers of log_cpu_state() do their own checks
>> as per (1), but you could make an argument that we should switch
>> to (2) instead.
>
> I think (2) is better, we do the check in one place and that's it. All
> call to log_cpu_state are safe. And as you said, it's already the way
> qemu_log and qemu_log_vprintf work.

Yeah, it seems reasonable to me. I think your commit message could
be better though, since you're actually kind of changing an API
here, not just fixing a segfault bug.

-- PMM

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Andreas Färber]

Am 15.05.2012 18:08, schrieb Fabien Chouteau:
> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>> Do not call cpu_dump_state if logfile is NULL.
>>
>> And where is log_cpu_state() being called from? Its caller is passing
>> NULL already then.
>>
> 
> No, logfile is a global variable. log_cpu_state() takes only CPUState
> and flags parameters.

Ah, I see now that f is a different f here, logfile becomes
log_cpu_state()'s f. Unfortunate naming.

Your fix looks OK then but I would recommend turning it into a static
inline function to avoid the line breaks.

Andreas

>>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>>> ---
>>>  qemu-log.h |    7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qemu-log.h b/qemu-log.h
>>> index fccfb110..2cd5ffa 100644
>>> --- a/qemu-log.h
>>> +++ b/qemu-log.h
>>> @@ -51,7 +51,12 @@ extern int loglevel;
>>>  /* Special cases: */
>>>  
>>>  /* cpu_dump_state() logging functions: */
>>> -#define log_cpu_state(env, f) cpu_dump_state((env), logfile, fprintf, (f));
>>> +#define log_cpu_state(env, f)                          \
>>> +do {                                                   \
>>> +    if (logfile != NULL) {                             \
>>> +        cpu_dump_state((env), logfile, fprintf, (f));  \
>>> +    }                                                  \
>>> + } while (0)
>>>  #define log_cpu_state_mask(b, env, f) do {           \
>>>        if (loglevel & (b)) log_cpu_state((env), (f)); \
>>>    } while (0)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Fabien Chouteau]

On 05/16/2012 05:50 AM, Andreas Färber wrote:
> Am 15.05.2012 18:08, schrieb Fabien Chouteau:
>> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>>> Do not call cpu_dump_state if logfile is NULL.
>>>
>>> And where is log_cpu_state() being called from? Its caller is passing
>>> NULL already then.
>>>
>>
>> No, logfile is a global variable. log_cpu_state() takes only CPUState
>> and flags parameters.
> 
> Ah, I see now that f is a different f here, logfile becomes
> log_cpu_state()'s f. Unfortunate naming.
> 
> Your fix looks OK then but I would recommend turning it into a static
> inline function to avoid the line breaks.
> 

In this case I can rewrite all the macros in qemu-log.h to static inline.

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Fabien Chouteau]

On 05/16/2012 10:29 AM, Fabien Chouteau wrote:
> On 05/16/2012 05:50 AM, Andreas Färber wrote:
>> Am 15.05.2012 18:08, schrieb Fabien Chouteau:
>>> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>>>> Do not call cpu_dump_state if logfile is NULL.
>>>>
>>>> And where is log_cpu_state() being called from? Its caller is passing
>>>> NULL already then.
>>>>
>>>
>>> No, logfile is a global variable. log_cpu_state() takes only CPUState
>>> and flags parameters.
>>
>> Ah, I see now that f is a different f here, logfile becomes
>> log_cpu_state()'s f. Unfortunate naming.
>>
>> Your fix looks OK then but I would recommend turning it into a static
>> inline function to avoid the line breaks.
>>
> 
> In this case I can rewrite all the macros in qemu-log.h to static inline.
> 

This is more complex than expected...

 1 - GCC rejects inlined variadic functions

 2 - Moving from macro to inline implies use of types defined in cpu.h
 (target_ulong, CPUArchState...), which I cannot include because
 qemu-log.h is used in tools (i.e.  without cpu.h).

Conclusion: unless someone volunteer for a massive restructuring of
qemu-log we have to keep the marcro for log_cpu_state.

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH] Avoid segfault in cpu_dump_state

[Fabien Chouteau]

On 05/16/2012 03:39 PM, Fabien Chouteau wrote:
> On 05/16/2012 10:29 AM, Fabien Chouteau wrote:
>> On 05/16/2012 05:50 AM, Andreas Färber wrote:
>>> Am 15.05.2012 18:08, schrieb Fabien Chouteau:
>>>> On 05/15/2012 03:31 PM, Andreas Färber wrote:
>>>>> Am 15.05.2012 11:39, schrieb Fabien Chouteau:
>>>>>> Do not call cpu_dump_state if logfile is NULL.
>>>>>
>>>>> And where is log_cpu_state() being called from? Its caller is passing
>>>>> NULL already then.
>>>>>
>>>>
>>>> No, logfile is a global variable. log_cpu_state() takes only CPUState
>>>> and flags parameters.
>>>
>>> Ah, I see now that f is a different f here, logfile becomes
>>> log_cpu_state()'s f. Unfortunate naming.
>>>
>>> Your fix looks OK then but I would recommend turning it into a static
>>> inline function to avoid the line breaks.
>>>
>>
>> In this case I can rewrite all the macros in qemu-log.h to static inline.
>>
> 
> This is more complex than expected...
> 
>  1 - GCC rejects inlined variadic functions
> 
>  2 - Moving from macro to inline implies use of types defined in cpu.h
>  (target_ulong, CPUArchState...), which I cannot include because
>  qemu-log.h is used in tools (i.e.  without cpu.h).
> 
> Conclusion: unless someone volunteer for a massive restructuring of
> qemu-log we have to keep the marcro for log_cpu_state.
> 

So, are we good with the second patch?

-- 
Fabien Chouteau