Patchwork Avoid segfault in cpu_dump_state

login
register
mail settings
Submitter Fabien Chouteau
Date May 15, 2012, 9:39 a.m.
Message ID <1337074749-24189-1-git-send-email-chouteau@adacore.com>
Download mbox | patch
Permalink /patch/159279/
State New
Headers show

Comments

Fabien Chouteau - May 15, 2012, 9:39 a.m.
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(-)
Andreas Färber - May 15, 2012, 1:31 p.m.
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)
Fabien Chouteau - May 15, 2012, 4:08 p.m.
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)
>
Peter Maydell - May 15, 2012, 4:20 p.m.
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
Fabien Chouteau - May 15, 2012, 4:45 p.m.
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.
Peter Maydell - May 15, 2012, 6:04 p.m.
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
Andreas Färber - May 16, 2012, 3:50 a.m.
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)
Fabien Chouteau - May 16, 2012, 8:29 a.m.
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 - May 16, 2012, 1:39 p.m.
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 - May 23, 2012, 3:43 p.m.
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?
Alexander Graf - May 30, 2012, 7:58 a.m.
On 23.05.2012, at 17:43, Fabien Chouteau wrote:

> 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?

No reply, so I assume that's a yes. Applied it to ppc-next :). Thanks a lot!


Alex
Fabien Chouteau - May 30, 2012, 8:07 a.m.
On 05/30/2012 09:58 AM, Alexander Graf wrote:
> 
> On 23.05.2012, at 17:43, Fabien Chouteau wrote:
> 
>> 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?
> 
> No reply, so I assume that's a yes. Applied it to ppc-next :). Thanks a lot!
> 

You're welcome!

Thanks,
Andreas Färber - May 30, 2012, 11:19 a.m.
Am 30.05.2012 09:58, schrieb Alexander Graf:
> 
> On 23.05.2012, at 17:43, Fabien Chouteau wrote:
> 
>> 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?
> 
> No reply, so I assume that's a yes.

I'm okay with it if there's no better way. Didn't find the time to
investigate myself.

Andreas

> Applied it to ppc-next :). Thanks a lot!
> 
> 
> Alex

Patch

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)