[Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out

[Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Spotted by Coverity:

(8) Event freed_arg:  "fclose(FILE *)" frees "wav->f".
(9) Event cond_true:  Condition "fclose(wav->f)", taking true branch
Also see events:  [pass_freed_arg]

212         if (fclose (wav->f))  {
(10) Event pass_freed_arg:  Passing freed pointer "wav->f" as an argument
to function "AUD_log(char const *, char const *, ...)".
Also see events:  [freed_arg]

213             dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
214                    wav->f, strerror (errno));

Drop the whole message, we can't do much when it happen after all.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 audio/wavaudio.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/audio/wavaudio.c b/audio/wavaudio.c
index 6846a1a..b81fdf9 100644
--- a/audio/wavaudio.c
+++ b/audio/wavaudio.c
@@ -209,10 +209,7 @@ static void wav_fini_out (HWVoiceOut *hw)
     }
 
  doclose:
-    if (fclose (wav->f))  {
-        dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
-               wav->f, strerror (errno));
-    }
+    fclose(wav->f);
     wav->f = NULL;
 
     g_free (wav->pcm_buf);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out

[Markus Armbruster]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> Spotted by Coverity:
>
> (8) Event freed_arg:  "fclose(FILE *)" frees "wav->f".
> (9) Event cond_true:  Condition "fclose(wav->f)", taking true branch
> Also see events:  [pass_freed_arg]
>
> 212         if (fclose (wav->f))  {
> (10) Event pass_freed_arg:  Passing freed pointer "wav->f" as an argument
> to function "AUD_log(char const *, char const *, ...)".
> Also see events:  [freed_arg]
>
> 213             dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
> 214                    wav->f, strerror (errno));

False positive, because dolog() doesn't dereference wav->f, it only
prints it.

> Drop the whole message, we can't do much when it happen after all.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  audio/wavaudio.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/audio/wavaudio.c b/audio/wavaudio.c
> index 6846a1a..b81fdf9 100644
> --- a/audio/wavaudio.c
> +++ b/audio/wavaudio.c
> @@ -209,10 +209,7 @@ static void wav_fini_out (HWVoiceOut *hw)
>      }
>  
>   doclose:
> -    if (fclose (wav->f))  {
> -        dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
> -               wav->f, strerror (errno));
> -    }
> +    fclose(wav->f);
>      wav->f = NULL;
>  
>      g_free (wav->pcm_buf);

I'm afraid this is not an improvement.

Your patch makes the code ignore fclose() failure silently.  This is a
common mistake.  fclose() failure after write can mean data loss, and
the user certainly needs to know about that.

Re: [Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out

[Peter Maydell]

On 13 June 2014 08:15, Markus Armbruster <armbru@redhat.com> wrote:
> <arei.gonglei@huawei.com> writes:
>
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Spotted by Coverity:
>>
>> (8) Event freed_arg:  "fclose(FILE *)" frees "wav->f".
>> (9) Event cond_true:  Condition "fclose(wav->f)", taking true branch
>> Also see events:  [pass_freed_arg]
>>
>> 212         if (fclose (wav->f))  {
>> (10) Event pass_freed_arg:  Passing freed pointer "wav->f" as an argument
>> to function "AUD_log(char const *, char const *, ...)".
>> Also see events:  [freed_arg]
>>
>> 213             dolog ("wav_fini_out: fclose %p failed\nReason: %s\n",
>> 214                    wav->f, strerror (errno));
>
> False positive, because dolog() doesn't dereference wav->f, it only
> prints it.

Yep, I pointed that out last time around...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out

[Paolo Bonzini]

Il 13/06/2014 09:15, Markus Armbruster ha scritto:
> I'm afraid this is not an improvement.
>
> Your patch makes the code ignore fclose() failure silently.  This is a
> common mistake.  fclose() failure after write can mean data loss, and
> the user certainly needs to know about that.

If you want that, the best solution is to first fflush() and then 
fclose().  Then you're sure that the fclose() doesn't cause data loss 
and you can safely ignore its result (it shouldn't fail).

Paolo

Re: [Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out

[Gerd Hoffmann]

On Fr, 2014-06-13 at 09:47 +0200, Paolo Bonzini wrote:
> Il 13/06/2014 09:15, Markus Armbruster ha scritto:
> > I'm afraid this is not an improvement.
> >
> > Your patch makes the code ignore fclose() failure silently.  This is a
> > common mistake.  fclose() failure after write can mean data loss, and
> > the user certainly needs to know about that.
> 
> If you want that, the best solution is to first fflush() and then 
> fclose().

Agree.  If you really care you'll go flush/sync before close, so you
still have a valid file handle when you see the failure.

Question is do we really need that here?  This isn't your virtual disk,
it's just a few audio samples which might get lost ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out

[Paolo Bonzini]

Il 13/06/2014 09:59, Gerd Hoffmann ha scritto:
> On Fr, 2014-06-13 at 09:47 +0200, Paolo Bonzini wrote:
>> Il 13/06/2014 09:15, Markus Armbruster ha scritto:
>>> I'm afraid this is not an improvement.
>>>
>>> Your patch makes the code ignore fclose() failure silently.  This is a
>>> common mistake.  fclose() failure after write can mean data loss, and
>>> the user certainly needs to know about that.
>>
>> If you want that, the best solution is to first fflush() and then
>> fclose().
>
> Agree.  If you really care you'll go flush/sync before close, so you
> still have a valid file handle when you see the failure.
>
> Question is do we really need that here?  This isn't your virtual disk,
> it's just a few audio samples which might get lost ...

I think sticking to fflush+fclose is a good idea anyway, if only for 
discipline.  If Gonglei wants to do that, why not. :)

Otherwise, it's easy to mark the bug as intentional in Coverity.

Paolo

Re: [Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Friday, June 13, 2014 4:18 PM
> To: Gerd Hoffmann
> Cc: Markus Armbruster; Gonglei (Arei); qemu-devel@nongnu.org;
> Huangweidong (C); Luonengjun; stefanha@redhat.com
> Subject: Re: [Qemu-devel] [PATCH] audio: Fix using freed pointer in wav_fini_out
> 
> Il 13/06/2014 09:59, Gerd Hoffmann ha scritto:
> > On Fr, 2014-06-13 at 09:47 +0200, Paolo Bonzini wrote:
> >> Il 13/06/2014 09:15, Markus Armbruster ha scritto:
> >>> I'm afraid this is not an improvement.
> >>>
> >>> Your patch makes the code ignore fclose() failure silently.  This is a
> >>> common mistake.  fclose() failure after write can mean data loss, and
> >>> the user certainly needs to know about that.
> >>
> >> If you want that, the best solution is to first fflush() and then
> >> fclose().
> >
> > Agree.  If you really care you'll go flush/sync before close, so you
> > still have a valid file handle when you see the failure.
> >
> > Question is do we really need that here?  This isn't your virtual disk,
> > it's just a few audio samples which might get lost ...
> 
> I think sticking to fflush+fclose is a good idea anyway, if only for
> discipline.  If Gonglei wants to do that, why not. :)
> 
> Otherwise, it's easy to mark the bug as intentional in Coverity.
> 
> Paolo

Thanks, you guys. 

Actually I search the whole qemu project before I post the patch,
and I find most module are just simply close the file handle, 
so I follow them.

For this issue, if you think we should stick to flush+fclose is a good idea,
I'd like to post another patch. I'm fine. Thanks.


Best regards,
-Gonglei