[PATCH] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[PATCH] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Li Zhong]

This patch tries to fix CID 997012, 997013 and 997014 reported by Coverity scan,
as suggested by sekharan.

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 repair/sb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/repair/sb.c b/repair/sb.c
index aa550e3..7abf47c 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -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);
 
-- 
1.8.1.4



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Mark Tinguely]

On 09/22/13 01:01, Li Zhong wrote:
> This patch tries to fix CID 997012, 997013 and 997014 reported by Coverity scan,
> as suggested by sekharan.
>
> Signed-off-by: Li Zhong<zhong@linux.vnet.ibm.com>
> ---


> @@ -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;
> +	}

>

Looks good. list, sb and check could have been allocated at this point.

Isn't the list been added to before the conditional in the for loop?:

         list = add_geo(list, &geo, sb_index);

         /*
          * grab N secondaries.  check them off as we get them
          * so we only process each one once
          */
         for (round = 0; round < skip; round++)  {

...
                         if (get_sb(sb, off, size, agno) == XR_EOF)  {
                                 retval = 1;
                                 goto out;
				^^^^^^^^^ out_free_list?
                         }

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Li Zhong]

This patch tries to fix CID 997012, 997013 and 997014 reported by Coverity scan,
as suggested by sekharan.

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.

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 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);
-- 
1.8.1.4


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Mark Tinguely]

On 09/25/13 02:32, Li Zhong wrote:
> This patch tries to fix CID 997012, 997013 and 997014 reported by Coverity scan,
> as suggested by sekharan.
>
> 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.
>
> Signed-off-by: Li Zhong<zhong@linux.vnet.ibm.com>
> ---

Thank-you, that looks great.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Eric Sandeen]

On 9/25/13 2:32 AM, Li Zhong wrote:
> This patch tries to fix CID 997012, 997013 and 997014 reported by Coverity scan,
> as suggested by sekharan.
> 
> 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.

Fix itself looks good, thanks!  Love to see the scan numbers change
for the better.  ;)

Nitpicks, though: Patch changelogs usually goes below the "---" so
the history of trial and error isn't in the commit log.  Not that big
a deal, it's just convention as mentioned in the kernel SubmittingPatches
doc:

> The "---" marker line serves the essential purpose of marking for patch
> handling tools where the changelog message ends.
> 
> One good use for the additional comments after the "---" marker is for
> a diffstat, to show what files have changed, and the number of
> inserted and deleted lines per file.  A diffstat is especially useful
> on bigger patches.  Other comments relevant only to the moment or the
> maintainer, not suitable for the permanent changelog, should also go
> here.  A good example of such comments might be "patch changelogs"
> which describe what has changed between the v1 and v2 version of the
> patch.


And since we're on the topic of commit messages lately, this one could
be improved too I think.

"CID 997012" won't mean anything to a reader in the future.  It'd be
better to describe what you're fixing on its own terms.  Something like:

===
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.
===

On the one hand, the fix isn't that complicated so it probably speaks for
itself.  But it was complicated enough to warrant discussion & V2 on the list,
so probably worth including that detail in the final changelog.


Also, in looking at this, I wonder if there's another minor buglet.

in phase1.c, we turn the return value from verify_set_primary_sb() into
an error string via err_string(rval).  This handles the various
error returns such as XR_INSUFF_SEC_SB, XR_EOF, etc.  But in the 2nd
case above (get_sb failure), it simply returns "1" which will be interpreted
as XR_BAD_MAGIC ("bad magic number").

get_sb() actually returns several XR_* values, so we should probably capture
it and use that return value?  That'd be a different patch though.

I guess the comment for verify_set_primary_sb() would be changed
then too, now it says:

 * returns 1 if bad, 0 if ok

but today we actually return 0, 1, or XR_INSUFF_SEC_SB.

Not that big a deal, but it seems like the error returns, their handling,
and associated comments aren't quite consistent.

Thanks,
-Eric


> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> ---
>  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);
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Li Zhong]

On Wed, 2013-09-25 at 09:28 -0500, Eric Sandeen wrote:
> On 9/25/13 2:32 AM, Li Zhong wrote:
> > This patch tries to fix CID 997012, 997013 and 997014 reported by Coverity scan,
> > as suggested by sekharan.
> > 
> > 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.
> 
> Fix itself looks good, thanks!  Love to see the scan numbers change
> for the better.  ;)

Thank you for all the education below. :)

> Nitpicks, though: Patch changelogs usually goes below the "---" so
> the history of trial and error isn't in the commit log.  Not that big
> a deal, it's just convention as mentioned in the kernel SubmittingPatches
> doc:

I think I'll practice it with a 3rd verion.

> 
> > The "---" marker line serves the essential purpose of marking for patch
> > handling tools where the changelog message ends.
> > 
> > One good use for the additional comments after the "---" marker is for
> > a diffstat, to show what files have changed, and the number of
> > inserted and deleted lines per file.  A diffstat is especially useful
> > on bigger patches.  Other comments relevant only to the moment or the
> > maintainer, not suitable for the permanent changelog, should also go
> > here.  A good example of such comments might be "patch changelogs"
> > which describe what has changed between the v1 and v2 version of the
> > patch.
> 
> 
> And since we're on the topic of commit messages lately, this one could
> be improved too I think.
> 
> "CID 997012" won't mean anything to a reader in the future.  It'd be
> better to describe what you're fixing on its own terms.  Something like:
> 
> ===
> 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.
> ===
> 
> On the one hand, the fix isn't that complicated so it probably speaks for
> itself.  But it was complicated enough to warrant discussion & V2 on the list,
> so probably worth including that detail in the final changelog.

OK, I'll use the above as the changelog.

> 
> 
> Also, in looking at this, I wonder if there's another minor buglet.
> 
> in phase1.c, we turn the return value from verify_set_primary_sb() into
> an error string via err_string(rval).  This handles the various
> error returns such as XR_INSUFF_SEC_SB, XR_EOF, etc.  But in the 2nd
> case above (get_sb failure), it simply returns "1" which will be interpreted
> as XR_BAD_MAGIC ("bad magic number").
> 
> get_sb() actually returns several XR_* values, so we should probably capture
> it and use that return value?  That'd be a different patch though.
> 
> I guess the comment for verify_set_primary_sb() would be changed
> then too, now it says:
> 
>  * returns 1 if bad, 0 if ok
> 
> but today we actually return 0, 1, or XR_INSUFF_SEC_SB.
> 
> Not that big a deal, but it seems like the error returns, their handling,
> and associated comments aren't quite consistent.

I'll try to make another patch for the above issue.
Thanks, Zhong

> 
> Thanks,
> -Eric
> 
> 
> > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> > ---
> >  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);
> > 
> 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH v3 1/2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Li Zhong]

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@linux.vnet.ibm.com>
---
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 "---".

 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);
-- 
1.8.1.4



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfsprogs: fix return value of verify_set_primary_sb()

[Li Zhong]

If get_sb() fails because of EOF, it will return with retval 1, which will
then be interpreted as XR_BAD_MAGIC("bad magic number") in phase1() when
warning the user.

This patch fix it by using XR_EOF here, so it would be interpreted correctly.
Also change the associated comments about the return value.

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 repair/sb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/repair/sb.c b/repair/sb.c
index d34d7a2..2e35a4c 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -665,7 +665,7 @@ get_sb_geometry(fs_geometry_t *geo, xfs_sb_t *sbp)
  * primary and compare the geometries in the secondaries against
  * the geometry indicated by the primary.
  *
- * returns 1 if bad, 0 if ok
+ * returns 0 if ok, else error code (XR_EOF, XR_INSUFF_SEC_SB, etc).
  */
 int
 verify_set_primary_sb(xfs_sb_t		*rsb,
@@ -732,7 +732,7 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
 			checked[agno] = 1;
 
 			if (get_sb(sb, off, size, agno) == XR_EOF)  {
-				retval = 1;
+				retval = XR_EOF;
 				goto out_free_list;
 			}
 
-- 
1.8.1.4



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfsprogs: fix return value of verify_set_primary_sb()

[Eric Sandeen]

On 9/26/13 1:48 AM, Li Zhong wrote:
> If get_sb() fails because of EOF, it will return with retval 1, which will
> then be interpreted as XR_BAD_MAGIC("bad magic number") in phase1() when
> warning the user.
> 
> This patch fix it by using XR_EOF here, so it would be interpreted correctly.
> Also change the associated comments about the return value.
> 
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>

I think this is OK.  I had originally thought we should return whatever XR_*
code get_sb() returned, but I wasn't thinking straight; we ignore those
other codes, and only stop if we see XR_EOF.

So this is fine as far as it goes; I noticed something else in the process
though so I'll send a patch for that.  :)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  repair/sb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/repair/sb.c b/repair/sb.c
> index d34d7a2..2e35a4c 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -665,7 +665,7 @@ get_sb_geometry(fs_geometry_t *geo, xfs_sb_t *sbp)
>   * primary and compare the geometries in the secondaries against
>   * the geometry indicated by the primary.
>   *
> - * returns 1 if bad, 0 if ok
> + * returns 0 if ok, else error code (XR_EOF, XR_INSUFF_SEC_SB, etc).
>   */
>  int
>  verify_set_primary_sb(xfs_sb_t		*rsb,
> @@ -732,7 +732,7 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
>  			checked[agno] = 1;
>  
>  			if (get_sb(sb, off, size, agno) == XR_EOF)  {
> -				retval = 1;
> +				retval = XR_EOF;
>  				goto out_free_list;
>  			}
>  
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfsprogs: fix return value of verify_set_primary_sb()

[Rich Johnston]

This has been committed.

Thanks
--Rich

commit ae181820d03e19f145dbd058c0f8fa59fa18468c
Author: Li Zhong <zhong@linux.vnet.ibm.com>
Date:   Thu Sep 26 06:48:12 2013 +0000

     xfsprogs: fix return value of verify_set_primary_sb()

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 1/2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Eric Sandeen]

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@linux.vnet.ibm.com>
> ---
> 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@redhat.com>

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);
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 1/2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Li Zhong]

On Thu, 2013-09-26 at 09:31 -0500, Eric Sandeen wrote:
> 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@linux.vnet.ibm.com>
> > ---
> > 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@redhat.com>
> 
> alongside Mark's.

Ah, I missed that. Is it ok to just add it here in this mail? 

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

> 
> >  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);
> > 
> 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 1/2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Eric Sandeen]

> On Sep 26, 2013, at 10:05 PM, Li Zhong <zhong@linux.vnet.ibm.com> wrote:
> 
>> On Thu, 2013-09-26 at 09:31 -0500, Eric Sandeen wrote:
>>> 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@linux.vnet.ibm.com>
>>> ---
>>> 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@redhat.com>
>> 
>> alongside Mark's.
> 
> Ah, I missed that. Is it ok to just add it here in this mail? 
> 
> Reviewed-by: Mark Tinguely <tinguely@sgi.com>
> 
Sorry, I meant that for sgi but wasn't clear.  Sometimes I talk too much.  :)

Eric

>> 
>>> 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);
> 
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 1/2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Li Zhong]

On Thu, 2013-09-26 at 22:24 -0500, Eric Sandeen wrote:
> 
> > On Sep 26, 2013, at 10:05 PM, Li Zhong <zhong@linux.vnet.ibm.com> wrote:
> > 
> >> On Thu, 2013-09-26 at 09:31 -0500, Eric Sandeen wrote:
> >>> 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@linux.vnet.ibm.com>
> >>> ---
> >>> 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@redhat.com>
> >> 
> >> alongside Mark's.
> > 
> > Ah, I missed that. Is it ok to just add it here in this mail? 
> > 
> > Reviewed-by: Mark Tinguely <tinguely@sgi.com>
> > 
> Sorry, I meant that for sgi but wasn't clear.  Sometimes I talk too much.  :)

Oh, I see :)

Thanks, Zhong

> 
> Eric
> 
> >> 
> >>> 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);
> > 
> > 
> 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH v3 1/2] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Rich Johnston]

This has been committed.

Thanks
--Rich

commit 548c2e3e5b123266b4b89cc81bedd113442570b8
Author: Li Zhong <zhong@linux.vnet.ibm.com>
Date:   Thu Sep 26 06:45:32 2013 +0000

     [v3, 1/2] xfsprogs: fix potential memory leak in 
verify_set_primary_sb()

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsprogs: fix potential memory leak in verify_set_primary_sb()

[Li Zhong]

On Tue, 2013-09-24 at 13:59 -0500, Mark Tinguely wrote:
> On 09/22/13 01:01, Li Zhong wrote:
> > This patch tries to fix CID 997012, 997013 and 997014 reported by Coverity scan,
> > as suggested by sekharan.
> >
> > Signed-off-by: Li Zhong<zhong@linux.vnet.ibm.com>
> > ---
> 
> 
> > @@ -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;
> > +	}
> 
> >
> 
> Looks good. list, sb and check could have been allocated at this point.
> 
> Isn't the list been added to before the conditional in the for loop?:

See, I just sent a new version including below change.

Seems it is a Coverity scan bug not discovering this :)

Thanks, Zhong

> 
>          list = add_geo(list, &geo, sb_index);
> 
>          /*
>           * grab N secondaries.  check them off as we get them
>           * so we only process each one once
>           */
>          for (round = 0; round < skip; round++)  {
> 
> ...
>                          if (get_sb(sb, off, size, agno) == XR_EOF)  {
>                                  retval = 1;
>                                  goto out;
> 				^^^^^^^^^ out_free_list?
>                          }
> 
> --Mark.
> 


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs