* [PATCH] xfsprogs: fix resouce leak in longform_dir2_rebuild()
@ 2013-10-12 6:42 Li Zhong
2013-10-14 21:46 ` Dave Chinner
0 siblings, 1 reply; 5+ messages in thread
From: Li Zhong @ 2013-10-12 6:42 UTC (permalink / raw)
To: xfsprogs; +Cc: Chandra Seetharaman
coverity scan 997010 reported following leak in repair/phase6.c
1309 if (error) {
1310 do_warn(
1311 _("space reservation failed (%d), filesystem may be out of space\n"),
1312 error);
25. Breaking from loop
1313 break;
1314 }
......
1342 libxfs_trans_commit(tp,
1343 XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC);
1344 }
CID 997010 (#1 of 1): Resource leak (RESOURCE_LEAK)
26. leaked_storage: Variable "tp" going out of scope leaks the storage it points to.
1345}
Though not reported by coverity, it seems that there might be some entries in
flist which needs to be freed in the failure case below libxfs_dir_createname(),
so I also added a bmap cancel there.
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
repair/phase6.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/repair/phase6.c b/repair/phase6.c
index a4ad7a3..0d88ad2 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1310,6 +1310,8 @@ longform_dir2_rebuild(
do_warn(
_("space reservation failed (%d), filesystem may be out of space\n"),
error);
+ libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES |
+ XFS_TRANS_ABORT);
break;
}
@@ -1323,6 +1325,7 @@ longform_dir2_rebuild(
do_warn(
_("name create failed in ino %" PRIu64 " (%d), filesystem may be out of space\n"),
ino, error);
+ libxfs_bmap_cancel(&flist);
libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES |
XFS_TRANS_ABORT);
break;
--
1.8.1.4
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] xfsprogs: fix resouce leak in longform_dir2_rebuild() 2013-10-12 6:42 [PATCH] xfsprogs: fix resouce leak in longform_dir2_rebuild() Li Zhong @ 2013-10-14 21:46 ` Dave Chinner 2013-10-15 2:55 ` [PATCH v2] xfsprogs: fix resource " Li Zhong 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2013-10-14 21:46 UTC (permalink / raw) To: Li Zhong; +Cc: Chandra Seetharaman, xfsprogs On Sat, Oct 12, 2013 at 02:42:54PM +0800, Li Zhong wrote: > coverity scan 997010 reported following leak in repair/phase6.c > > 1309 if (error) { > 1310 do_warn( > 1311 _("space reservation failed (%d), filesystem may be out of space\n"), > 1312 error); > 25. Breaking from loop > 1313 break; > 1314 } > > ...... > > 1342 libxfs_trans_commit(tp, > 1343 XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC); > 1344 } > > CID 997010 (#1 of 1): Resource leak (RESOURCE_LEAK) > 26. leaked_storage: Variable "tp" going out of scope leaks the storage it points to. > 1345} > > Though not reported by coverity, it seems that there might be some entries in > flist which needs to be freed in the failure case below libxfs_dir_createname(), > so I also added a bmap cancel there. > > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> > --- > repair/phase6.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/repair/phase6.c b/repair/phase6.c > index a4ad7a3..0d88ad2 100644 > --- a/repair/phase6.c > +++ b/repair/phase6.c > @@ -1310,6 +1310,8 @@ longform_dir2_rebuild( > do_warn( > _("space reservation failed (%d), filesystem may be out of space\n"), > error); > + libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | > + XFS_TRANS_ABORT); As per the rest of the code in phase 6, the error handling here should call "res_failed()" as we can't sanely recover from an ENOSPC error during phase6. > break; > } > > @@ -1323,6 +1325,7 @@ longform_dir2_rebuild( > do_warn( > _("name create failed in ino %" PRIu64 " (%d), filesystem may be out of space\n"), > ino, error); > + libxfs_bmap_cancel(&flist); > libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | > XFS_TRANS_ABORT); > break; There's another case exactly the same in that function you missed. What you should probably do is stack the error handling cases at the end of the function like: .... return; out_bmap_cancel: libxfs_bmap_cancel(&flist); out_trans_cancel: libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT); return; } And convert all the error handling cases to jump to the appropriate error handler. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] xfsprogs: fix resource leak in longform_dir2_rebuild() 2013-10-14 21:46 ` Dave Chinner @ 2013-10-15 2:55 ` Li Zhong 2013-10-15 21:39 ` Dave Chinner 2013-10-18 19:33 ` Rich Johnston 0 siblings, 2 replies; 5+ messages in thread From: Li Zhong @ 2013-10-15 2:55 UTC (permalink / raw) To: Dave Chinner; +Cc: Chandra Seetharaman, xfsprogs coverity scan 997010 reported following leak: 1309 if (error) { 1310 do_warn( 1311 _("space reservation failed (%d), filesystem may be out of space\n"), 1312 error); 25. Breaking from loop 1313 break; 1314 } ...... 1342 libxfs_trans_commit(tp, 1343 XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC); 1344 } CID 997010 (#1 of 1): Resource leak (RESOURCE_LEAK) 26. leaked_storage: Variable "tp" going out of scope leaks the storage it points to. 1345} Though not reported by coverity, it seems that there might be some entries in flist which needs to be freed in the failure case below libxfs_dir_createname(), and libxfs_bunmapi(). The fix cleans up the code by stacking the error handling at the end of the function, and jumping to the error handler label for the above cases. (fail directly by calling res_failed() for reservation failure.) Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> --- v2: as Dave suggested, use res_failed() for libxfs_trans_reserve() adding bmap cancel for one missed case using error handling lables to clean up the code repair/phase6.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/repair/phase6.c b/repair/phase6.c index a4ad7a3..6b3fb09 100644 --- a/repair/phase6.c +++ b/repair/phase6.c @@ -1280,9 +1280,7 @@ longform_dir2_rebuild( &firstblock, &flist, &done); if (error) { do_warn(_("xfs_bunmapi failed -- error - %d\n"), error); - libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | - XFS_TRANS_ABORT); - return; + goto out_bmap_cancel; } ASSERT(done); @@ -1306,12 +1304,8 @@ longform_dir2_rebuild( nres = XFS_CREATE_SPACE_RES(mp, p->name.len); error = libxfs_trans_reserve(tp, &M_RES(mp)->tr_create, nres, 0); - if (error) { - do_warn( - _("space reservation failed (%d), filesystem may be out of space\n"), - error); - break; - } + if (error) + res_failed(error); libxfs_trans_ijoin(tp, ip, 0); libxfs_trans_ihold(tp, ip); @@ -1323,9 +1317,7 @@ longform_dir2_rebuild( do_warn( _("name create failed in ino %" PRIu64 " (%d), filesystem may be out of space\n"), ino, error); - libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | - XFS_TRANS_ABORT); - break; + goto out_bmap_cancel; } error = libxfs_bmap_finish(&tp, &flist, &committed); @@ -1333,15 +1325,19 @@ _("name create failed in ino %" PRIu64 " (%d), filesystem may be out of space\n" do_warn( _("bmap finish failed (%d), filesystem may be out of space\n"), error); - libxfs_bmap_cancel(&flist); - libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | - XFS_TRANS_ABORT); - break; + goto out_bmap_cancel; } libxfs_trans_commit(tp, XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC); } + + return; + +out_bmap_cancel: + libxfs_bmap_cancel(&flist); + libxfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT); + return; } -- 1.8.1.4 _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] xfsprogs: fix resource leak in longform_dir2_rebuild() 2013-10-15 2:55 ` [PATCH v2] xfsprogs: fix resource " Li Zhong @ 2013-10-15 21:39 ` Dave Chinner 2013-10-18 19:33 ` Rich Johnston 1 sibling, 0 replies; 5+ messages in thread From: Dave Chinner @ 2013-10-15 21:39 UTC (permalink / raw) To: Li Zhong; +Cc: Chandra Seetharaman, xfsprogs On Tue, Oct 15, 2013 at 10:55:31AM +0800, Li Zhong wrote: > coverity scan 997010 reported following leak: > > 1309 if (error) { > 1310 do_warn( > 1311 _("space reservation failed (%d), filesystem may be out of space\n"), > 1312 error); > 25. Breaking from loop > 1313 break; > 1314 } > > ...... > > 1342 libxfs_trans_commit(tp, > 1343 XFS_TRANS_RELEASE_LOG_RES|XFS_TRANS_SYNC); > 1344 } > > CID 997010 (#1 of 1): Resource leak (RESOURCE_LEAK) > 26. leaked_storage: Variable "tp" going out of scope leaks the storage it points to. > 1345} > > Though not reported by coverity, it seems that there might be some entries in > flist which needs to be freed in the failure case below libxfs_dir_createname(), > and libxfs_bunmapi(). > > The fix cleans up the code by stacking the error handling at the end of the > function, and jumping to the error handler label for the above cases. (fail > directly by calling res_failed() for reservation failure.) > > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> Looks much better. Thanks! :) Reviewed-by: Dave Chinner <dchinner@redhat.com> -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] xfsprogs: fix resource leak in longform_dir2_rebuild() 2013-10-15 2:55 ` [PATCH v2] xfsprogs: fix resource " Li Zhong 2013-10-15 21:39 ` Dave Chinner @ 2013-10-18 19:33 ` Rich Johnston 1 sibling, 0 replies; 5+ messages in thread From: Rich Johnston @ 2013-10-18 19:33 UTC (permalink / raw) To: Li Zhong, Dave Chinner; +Cc: Chandra Seetharaman, xfsprogs This has been committed. Thanks --Rich commit 14e36e34ed975b1ffbbcbc18a7743a3a4859be3d Author: Li Zhong <zhong@linux.vnet.ibm.com> Date: Tue Oct 15 02:55:31 2013 +0000 xfsprogs: fix resource leak in longform_dir2_rebuild() _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-18 19:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-12 6:42 [PATCH] xfsprogs: fix resouce leak in longform_dir2_rebuild() Li Zhong 2013-10-14 21:46 ` Dave Chinner 2013-10-15 2:55 ` [PATCH v2] xfsprogs: fix resource " Li Zhong 2013-10-15 21:39 ` Dave Chinner 2013-10-18 19:33 ` Rich Johnston
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox