[Ocfs2-tools-devel] [PATCH 1/2] ocfs2-tools: fix a double-free bug caused by ocfs2_bitmap_free
Joseph Qi
joseph.qi at huawei.com
Tue Mar 1 00:59:37 PST 2016
On 2016/3/1 16:48, piaojun wrote:
> When I am using fsck.ocfs2 to fix a gd problem in ocfs2, a double-free
> bug happens. The detail of the bug is as below:
> main()->
> o2fsck_pass1()->
> write_cluster_alloc()->
> ocfs2_load_chain_allocator()->
> ocfs2_bitmap_read()->ocfs2_bitmap_free()->
> ocfs2_free_cached_inode()
>
> In ocfs2_load_chain_allocator(), ocfs2_bitmap_read() fails and goes
> into ocfs2_bitmap_free(). But in ocfs2_bitmap_free(), the
> cinode->ci_chains is freed but not assigned to NULL. Then
> ocfs2_load_chain_allocator() fails and goto out to
> ocfs2_free_cached_inode. At last, cinode->ci_chains is freed twice.
>
> So the argument passed into ocfs2_bitmap_free should be assigned to
> NULL by ocfs2_free rather than the parameter bitmap.
>
> Signed-off-by: Jun Piao <piaojun at huawei.com>
Looks good, thanks.
Reviewed-by: Joseph Qi <joseph.qi at huawei.com>
> ---
> extras/find_dup_extents.c | 4 ++--
> fsck.ocfs2/fsck.c | 8 ++++----
> fsck.ocfs2/icount.c | 2 +-
> fsck.ocfs2/journal.c | 2 +-
> fsck.ocfs2/pass0.c | 4 ++--
> include/ocfs2/ocfs2.h | 2 +-
> libocfs2/bitmap.c | 20 ++++++++++----------
> libocfs2/cached_inode.c | 4 ++--
> libocfs2/chainalloc.c | 4 ++--
> 9 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/extras/find_dup_extents.c b/extras/find_dup_extents.c
> index 1f8fdcc..f33d8ed 100644
> --- a/extras/find_dup_extents.c
> +++ b/extras/find_dup_extents.c
> @@ -270,8 +270,8 @@ int main(int argc, char *argv[])
> if (!ret && we.has_dups)
> ret = run_scan(&we, 1);
>
> - ocfs2_bitmap_free(we.extent_map);
> - ocfs2_bitmap_free(we.dup_map);
> + ocfs2_bitmap_free(&we.extent_map);
> + ocfs2_bitmap_free(&we.dup_map);
>
> out_close:
> ret = ocfs2_close(fs);
> diff --git a/fsck.ocfs2/fsck.c b/fsck.ocfs2/fsck.c
> index 4b7d4b3..561787f 100644
> --- a/fsck.ocfs2/fsck.c
> +++ b/fsck.ocfs2/fsck.c
> @@ -202,17 +202,17 @@ errcode_t o2fsck_state_reinit(ocfs2_filesys *fs,
> o2fsck_state *ost)
> {
> errcode_t ret;
>
> - ocfs2_bitmap_free(ost->ost_dir_inodes);
> + ocfs2_bitmap_free(&ost->ost_dir_inodes);
> ost->ost_dir_inodes = NULL;
>
> - ocfs2_bitmap_free(ost->ost_reg_inodes);
> + ocfs2_bitmap_free(&ost->ost_reg_inodes);
> ost->ost_reg_inodes = NULL;
>
> - ocfs2_bitmap_free(ost->ost_allocated_clusters);
> + ocfs2_bitmap_free(&ost->ost_allocated_clusters);
> ost->ost_allocated_clusters = NULL;
>
> if (ost->ost_duplicate_clusters) {
> - ocfs2_bitmap_free(ost->ost_duplicate_clusters);
> + ocfs2_bitmap_free(&ost->ost_duplicate_clusters);
> ost->ost_duplicate_clusters = NULL;
> }
>
> diff --git a/fsck.ocfs2/icount.c b/fsck.ocfs2/icount.c
> index 1fa7aab..68781cd 100644
> --- a/fsck.ocfs2/icount.c
> +++ b/fsck.ocfs2/icount.c
> @@ -232,7 +232,7 @@ void o2fsck_icount_free(o2fsck_icount *icount)
> struct rb_node *node;
> icount_node *in;
>
> - ocfs2_bitmap_free(icount->ic_single_bm);
> + ocfs2_bitmap_free(&icount->ic_single_bm);
> while((node = rb_first(&icount->ic_multiple_tree)) != NULL) {
> in = rb_entry(node, icount_node, in_node);
> rb_erase(node, &icount->ic_multiple_tree);
> diff --git a/fsck.ocfs2/journal.c b/fsck.ocfs2/journal.c
> index ff47633..bba8499 100644
> --- a/fsck.ocfs2/journal.c
> +++ b/fsck.ocfs2/journal.c
> @@ -722,7 +722,7 @@ out:
> if (buf)
> ocfs2_free(&buf);
> if (used_blocks)
> - ocfs2_bitmap_free(used_blocks);
> + ocfs2_bitmap_free(&used_blocks);
>
> return ret;
> }
> diff --git a/fsck.ocfs2/pass0.c b/fsck.ocfs2/pass0.c
> index fd1848b..3534214 100644
> --- a/fsck.ocfs2/pass0.c
> +++ b/fsck.ocfs2/pass0.c
> @@ -1293,9 +1293,9 @@ static errcode_t verify_bitmap_descs(o2fsck_state
> *ost,
>
> out:
> if (allowed)
> - ocfs2_bitmap_free(allowed);
> + ocfs2_bitmap_free(&allowed);
> if (forbidden)
> - ocfs2_bitmap_free(forbidden);
> + ocfs2_bitmap_free(&forbidden);
> return ret;
> }
>
> diff --git a/include/ocfs2/ocfs2.h b/include/ocfs2/ocfs2.h
> index d33a28d..5a05cc3 100644
> --- a/include/ocfs2/ocfs2.h
> +++ b/include/ocfs2/ocfs2.h
> @@ -626,7 +626,7 @@ errcode_t ocfs2_cluster_bitmap_new(ocfs2_filesys *fs,
> errcode_t ocfs2_block_bitmap_new(ocfs2_filesys *fs,
> const char *description,
> ocfs2_bitmap **ret_bitmap);
> -void ocfs2_bitmap_free(ocfs2_bitmap *bitmap);
> +void ocfs2_bitmap_free(ocfs2_bitmap **p_bitmap);
> errcode_t ocfs2_bitmap_set(ocfs2_bitmap *bitmap, uint64_t bitno,
> int *oldval);
> errcode_t ocfs2_bitmap_clear(ocfs2_bitmap *bitmap, uint64_t bitno,
> diff --git a/libocfs2/bitmap.c b/libocfs2/bitmap.c
> index e26bebb..9c2104e 100644
> --- a/libocfs2/bitmap.c
> +++ b/libocfs2/bitmap.c
> @@ -37,7 +37,7 @@
>
> /* The public API */
>
> -void ocfs2_bitmap_free(ocfs2_bitmap *bitmap)
> +void ocfs2_bitmap_free(ocfs2_bitmap **bitmap)
> {
> struct rb_node *node;
> struct ocfs2_bitmap_region *br;
> @@ -47,18 +47,18 @@ void ocfs2_bitmap_free(ocfs2_bitmap *bitmap)
> * it should have done it in destroy_notify. Same with the
> * private pointers.
> */
> - if (bitmap->b_ops->destroy_notify)
> - (*bitmap->b_ops->destroy_notify)(bitmap);
> + if ((*bitmap)->b_ops->destroy_notify)
> + (*(*bitmap)->b_ops->destroy_notify)(*bitmap);
>
> - while ((node = rb_first(&bitmap->b_regions)) != NULL) {
> + while ((node = rb_first(&(*bitmap)->b_regions)) != NULL) {
> br = rb_entry(node, struct ocfs2_bitmap_region, br_node);
>
> - rb_erase(&br->br_node, &bitmap->b_regions);
> + rb_erase(&br->br_node, &(*bitmap)->b_regions);
> ocfs2_bitmap_free_region(br);
> }
>
> - ocfs2_free(&bitmap->b_description);
> - ocfs2_free(&bitmap);
> + ocfs2_free(&(*bitmap)->b_description);
> + ocfs2_free(bitmap);
> }
>
> errcode_t ocfs2_bitmap_set(ocfs2_bitmap *bitmap, uint64_t bitno,
> @@ -907,14 +907,14 @@ errcode_t ocfs2_cluster_bitmap_new(ocfs2_filesys *fs,
> ret = ocfs2_bitmap_alloc_region(bitmap, bitoff, 0,
> alloc_bits, &br);
> if (ret) {
> - ocfs2_bitmap_free(bitmap);
> + ocfs2_bitmap_free(&bitmap);
> return ret;
> }
>
> ret = ocfs2_bitmap_insert_region(bitmap, br);
> if (ret) {
> ocfs2_bitmap_free_region(br);
> - ocfs2_bitmap_free(bitmap);
> + ocfs2_bitmap_free(&bitmap);
> return ret;
> }
>
> @@ -1241,7 +1241,7 @@ int main(int argc, char *argv[])
>
> run_test(bitmap);
>
> - ocfs2_bitmap_free(bitmap);
> + ocfs2_bitmap_free(&bitmap);
>
> out_close:
> ocfs2_close(fs);
> diff --git a/libocfs2/cached_inode.c b/libocfs2/cached_inode.c
> index 24ba324..8b7126b 100644
> --- a/libocfs2/cached_inode.c
> +++ b/libocfs2/cached_inode.c
> @@ -74,7 +74,7 @@ errcode_t ocfs2_free_cached_inode(ocfs2_filesys *fs,
> return OCFS2_ET_INVALID_ARGUMENT;
>
> if (cinode->ci_chains)
> - ocfs2_bitmap_free(cinode->ci_chains);
> + ocfs2_bitmap_free(&cinode->ci_chains);
>
> if (cinode->ci_inode)
> ocfs2_free(&cinode->ci_inode);
> @@ -106,7 +106,7 @@ errcode_t ocfs2_refresh_cached_inode(ocfs2_filesys *fs,
> ocfs2_cached_inode *cinode)
> {
> if (cinode->ci_chains) {
> - ocfs2_bitmap_free(cinode->ci_chains);
> + ocfs2_bitmap_free(&cinode->ci_chains);
> cinode->ci_chains = NULL;
> }
>
> diff --git a/libocfs2/chainalloc.c b/libocfs2/chainalloc.c
> index f886725..a866945 100644
> --- a/libocfs2/chainalloc.c
> +++ b/libocfs2/chainalloc.c
> @@ -433,7 +433,7 @@ errcode_t ocfs2_load_chain_allocator(ocfs2_filesys *fs,
> char name[256];
>
> if (cinode->ci_chains)
> - ocfs2_bitmap_free(cinode->ci_chains);
> + ocfs2_bitmap_free(&cinode->ci_chains);
>
> total_bits = (uint64_t)fs->fs_clusters *
> cinode->ci_inode->id2.i_chain.cl_bpc;
> @@ -454,7 +454,7 @@ errcode_t ocfs2_load_chain_allocator(ocfs2_filesys *fs,
> cinode, gb_blkno);
> ret = ocfs2_bitmap_read(cinode->ci_chains);
> if (ret) {
> - ocfs2_bitmap_free(cinode->ci_chains);
> + ocfs2_bitmap_free(&cinode->ci_chains);
> return ret;
> }
>
More information about the Ocfs2-tools-devel
mailing list