xfs
[Top] [All Lists]

Re: [PATCH v3 1/2] xfsprogs: fix potential memory leak in verify_set_pri

To: Li Zhong <zhong@xxxxxxxxxxxxxxxxxx>
Subject: Re: [PATCH v3 1/2] xfsprogs: fix potential memory leak in verify_set_primary_sb()
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Thu, 26 Sep 2013 09:31:25 -0500
Cc: xfsprogs <xfs@xxxxxxxxxxx>, Mark Tinguely <tinguely@xxxxxxx>, Chandra Seetharaman <sekharan@xxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1380177932.2983.11.camel@ThinkPad-T5421>
References: <1379829679.4089.2.camel@ThinkPad-T5421> <5241E125.7010902@xxxxxxx> <1380094327.2526.5.camel@ThinkPad-T5421> <5242F31B.4060902@xxxxxxxxxxx> <1380177932.2983.11.camel@ThinkPad-T5421>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/20130801 Thunderbird/17.0.8
On 9/26/13 1:45 AM, Li Zhong wrote:
> If verify_set_primary_sb() completes the secondary sb scanning loop with
> too few valid secondaries found (num_ok < num_sbs / 2), it will immediately
> return without freeing any of the previously allocated memory (variables
> sb, checked, and any items on the geo list).  This was reported by
> the Coverity scanner as CID 997012, 997013 and 997014.
> 
> Fix this by using the out_free_list: goto target for this error case.
> 
> Earlier, if get_sb() fails in the secondary scan loop, it goes to
> the out: target which does not free any items on the geo list.   Fix
> this by using the out_free_list: target as well, and remove the now-unused
> out: target.
> 
> Signed-off-by: Li Zhong <zhong@xxxxxxxxxxxxxxxxxx>
> ---
> v2: as Mark pointed out, out in the for loop before also needs list to
>  be freed. Also remove out lable as it is not referenced any more.
> v3: use a meaningful changlog from Eric, and hide the patch changlogs below 
> "---".

Thanks for that; you can add my:

Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>

alongside Mark's.

>  repair/sb.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/repair/sb.c b/repair/sb.c
> index aa550e3..d34d7a2 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -733,7 +733,7 @@ verify_set_primary_sb(xfs_sb_t            *rsb,
>  
>                       if (get_sb(sb, off, size, agno) == XR_EOF)  {
>                               retval = 1;
> -                             goto out;
> +                             goto out_free_list;
>                       }
>  
>                       if (verify_sb(sb, 0) == XR_OK)  {
> @@ -756,8 +756,10 @@ verify_set_primary_sb(xfs_sb_t           *rsb,
>       /*
>        * see if we have enough superblocks to bother with
>        */
> -     if (num_ok < num_sbs / 2)
> -             return(XR_INSUFF_SEC_SB);
> +     if (num_ok < num_sbs / 2) {
> +             retval = XR_INSUFF_SEC_SB;
> +             goto out_free_list;
> +     }
>  
>       current = get_best_geo(list);
>  
> @@ -841,7 +843,6 @@ verify_set_primary_sb(xfs_sb_t            *rsb,
>  
>  out_free_list:
>       free_geo(list);
> -out:
>       free(sb);
>       free(checked);
>       return(retval);
> 

<Prev in Thread] Current Thread [Next in Thread>