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

[Markus Armbruster]

<address@hidden> writes:

> From: Gonglei <address@hidden>
>
> 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 <address@hidden>
> Reviewed-by: Paolo Bonzini <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> ---
>  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.