* [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