Re: [PATCH] e2fsprogs: make ext2fs_free() to set the pointer to NULL

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 19, 2014 at 12:48:16PM +0100, Lukas Czerner wrote:
> Currently someone uses ext2fs_free() to free the ext2_filsys structure,
> he is also responsible to set the pointer to that structure to NULL,
> because ext2fs_free() is not able to do that.
> 
> This however is in contrast with ext2fs_free_mem() which requires as an
> argument pointer to the pointer and it will in fact set the pointer to
> NULL for us. This is probably the reason why majority of places where we
> use ext2fs_free() does not set the pointer to NULL afterwards.
> 
> Fix this by changing function ext2fs_free() so that it'll require
> pointer to the ext2_filsys as an argument and will be able to set the
> pointer to NULL for us.
> 
> I do not currently have any way to trigger the issue in recent
> e2fsprogs. Part of the reason is that there are some fixes which just
> obfuscates the real problem (for example
> 7ff040f30f0ff3bf5e2c832da3cb577e00a52d60) and the other part might be
> just coincidence.
> 
> I was however able to reproduce this with e2fsprogs 1.42.9 using the
> image in bug https://bugzilla.redhat.com/show_bug.cgi?id=997982. With
> this patch it fixes it. However I am not sure why it got fixed in the
> recent e2fsprogs since git bisect lands into the merge commit - however
> I think it's just a coincidence rather than targeted fix.
> 
> Signed-off-by: Lukas Czerner <lczerner@xxxxxxxxxx>
> ---
>  e2fsck/journal.c        |  2 +-
>  e2fsck/unix.c           | 12 ++++--------
>  lib/ext2fs/closefs.c    |  2 +-
>  lib/ext2fs/dupfs.c      |  2 +-
>  lib/ext2fs/ext2fs.h     |  2 +-
>  lib/ext2fs/freefs.c     | 10 ++++++++--
>  lib/ext2fs/initialize.c |  2 +-
>  lib/ext2fs/openfs.c     |  7 +++----
>  misc/tune2fs.c          |  2 +-
>  resize/online.c         |  2 +-
>  resize/resize2fs.c      |  4 ++--
>  11 files changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index a7b1150..ef964a0 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -965,7 +965,7 @@ errcode_t e2fsck_run_ext3_journal(e2fsck_t ctx)
>  		kbytes_written = stats->bytes_written >> 10;
>  
>  	ext2fs_mmp_stop(ctx->fs);
> -	ext2fs_free(ctx->fs);
> +	ext2fs_free(&ctx->fs);
>  	retval = ext2fs_open(ctx->filesystem_name, EXT2_FLAG_RW,
>  			     ctx->superblock, blocksize, io_ptr,
>  			     &ctx->fs);
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index 429f1cd..b4d23de 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -1051,10 +1051,8 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
>  		int blocksize;
>  		for (blocksize = EXT2_MIN_BLOCK_SIZE;
>  		     blocksize <= EXT2_MAX_BLOCK_SIZE; blocksize *= 2) {
> -			if (*ret_fs) {
> -				ext2fs_free(*ret_fs);
> -				*ret_fs = NULL;
> -			}
> +			if (*ret_fs)
> +				ext2fs_free(ret_fs);
>  			retval = ext2fs_open2(ctx->filesystem_name,
>  					      ctx->io_options, flags,
>  					      ctx->superblock, blocksize,
> @@ -1292,10 +1290,8 @@ restart:
>  			retval = retval2;
>  			goto failure;
>  		}
> -		if (fs->flags & EXT2_FLAG_NOFREE_ON_ERROR) {
> -			ext2fs_free(fs);
> -			fs = NULL;
> -		}
> +		if (fs->flags & EXT2_FLAG_NOFREE_ON_ERROR)
> +			ext2fs_free(&fs);
>  		if (!fs || (fs->group_desc_count > 1)) {
>  			log_out(ctx, _("%s: %s trying backup blocks...\n"),
>  				ctx->program_name,
> diff --git a/lib/ext2fs/closefs.c b/lib/ext2fs/closefs.c
> index c2eaec1..b0e515d 100644
> --- a/lib/ext2fs/closefs.c
> +++ b/lib/ext2fs/closefs.c
> @@ -490,7 +490,7 @@ errcode_t ext2fs_close2(ext2_filsys fs, int flags)
>  	if (retval)
>  		return retval;
>  
> -	ext2fs_free(fs);
> +	ext2fs_free(&fs);

Doesn't this change also require ext2fs_close2() take a (ext2_filsys *)?

If you're going to make an ABI change, you might as well fix the get/free_mem
ABIs to take (void **) since that's what they've been doing all along anyway.

--D

>  	return 0;
>  }
>  
> diff --git a/lib/ext2fs/dupfs.c b/lib/ext2fs/dupfs.c
> index 02721e1..7568d89 100644
> --- a/lib/ext2fs/dupfs.c
> +++ b/lib/ext2fs/dupfs.c
> @@ -115,7 +115,7 @@ errcode_t ext2fs_dup_handle(ext2_filsys src, ext2_filsys *dest)
>  	*dest = fs;
>  	return 0;
>  errout:
> -	ext2fs_free(fs);
> +	ext2fs_free(&fs);
>  	return retval;
>  
>  }
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index 7f7fd1f..a084888 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -1213,7 +1213,7 @@ extern char *ext2fs_find_block_device(dev_t device);
>  extern errcode_t ext2fs_sync_device(int fd, int flushb);
>  
>  /* freefs.c */
> -extern void ext2fs_free(ext2_filsys fs);
> +extern void ext2fs_free(ext2_filsys *fs);
>  extern void ext2fs_free_dblist(ext2_dblist dblist);
>  extern void ext2fs_badblocks_list_free(ext2_badblocks_list bb);
>  extern void ext2fs_u32_list_free(ext2_u32_list bb);
> diff --git a/lib/ext2fs/freefs.c b/lib/ext2fs/freefs.c
> index 89a157b..be7c677 100644
> --- a/lib/ext2fs/freefs.c
> +++ b/lib/ext2fs/freefs.c
> @@ -18,8 +18,14 @@
>  #include "ext2_fs.h"
>  #include "ext2fsP.h"
>  
> -void ext2fs_free(ext2_filsys fs)
> +/*
> + * Free ext2_filsys.  The 'fs_ptr' arg must point to a ext2_filsys
> + * which is the pointer to struct_ext2_filsys.
> + */
> +void ext2fs_free(ext2_filsys *fs_ptr)
>  {
> +	ext2_filsys fs = *fs_ptr;
> +
>  	if (!fs || (fs->magic != EXT2_ET_MAGIC_EXT2FS_FILSYS))
>  		return;
>  	if (fs->image_io != fs->io) {
> @@ -61,7 +67,7 @@ void ext2fs_free(ext2_filsys fs)
>  
>  	fs->magic = 0;
>  
> -	ext2fs_free_mem(&fs);
> +	ext2fs_free_mem(fs_ptr);
>  }
>  
>  /*
> diff --git a/lib/ext2fs/initialize.c b/lib/ext2fs/initialize.c
> index 75fbf8e..72ff090 100644
> --- a/lib/ext2fs/initialize.c
> +++ b/lib/ext2fs/initialize.c
> @@ -530,6 +530,6 @@ ipg_retry:
>  	return 0;
>  cleanup:
>  	free(buf);
> -	ext2fs_free(fs);
> +	ext2fs_free(&fs);
>  	return retval;
>  }
> diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
> index aba8a50..6a49083 100644
> --- a/lib/ext2fs/openfs.c
> +++ b/lib/ext2fs/openfs.c
> @@ -476,10 +476,9 @@ errcode_t ext2fs_open2(const char *name, const char *io_options,
>  
>  	return 0;
>  cleanup:
> -	if (flags & EXT2_FLAG_NOFREE_ON_ERROR)
> -		*ret_fs = fs;
> -	else
> -		ext2fs_free(fs);
> +	if (!(flags & EXT2_FLAG_NOFREE_ON_ERROR))
> +		ext2fs_free(&fs);
> +	*ret_fs = fs;
>  	return retval;
>  }
>  
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 8ff47d2..5a3f6db 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -2471,7 +2471,7 @@ retry_open:
>  			fprintf(stderr, "%s",
>  			     _("Couldn't find valid filesystem superblock.\n"));
>  
> -		ext2fs_free(fs);
> +		ext2fs_free(&fs);
>  		exit(1);
>  	}
>  	fs->default_bitmap_type = EXT2FS_BMAP64_RBTREE;
> diff --git a/resize/online.c b/resize/online.c
> index 46d86b0..afd5bb6 100644
> --- a/resize/online.c
> +++ b/resize/online.c
> @@ -290,7 +290,7 @@ errcode_t online_resize_fs(ext2_filsys fs, const char *mtpt,
>  		}
>  	}
>  
> -	ext2fs_free(new_fs);
> +	ext2fs_free(&new_fs);
>  	close(fd);
>  
>  	return 0;
> diff --git a/resize/resize2fs.c b/resize/resize2fs.c
> index 498cf99..6f19528 100644
> --- a/resize/resize2fs.c
> +++ b/resize/resize2fs.c
> @@ -208,7 +208,7 @@ errcode_t resize_fs(ext2_filsys fs, blk64_t *new_size, int flags,
>  
>  	rfs->flags = flags;
>  
> -	ext2fs_free(rfs->old_fs);
> +	ext2fs_free(&rfs->old_fs);
>  	if (rfs->itable_buf)
>  		ext2fs_free_mem(&rfs->itable_buf);
>  	if (rfs->reserve_blocks)
> @@ -221,7 +221,7 @@ errcode_t resize_fs(ext2_filsys fs, blk64_t *new_size, int flags,
>  
>  errout:
>  	if (rfs->new_fs)
> -		ext2fs_free(rfs->new_fs);
> +		ext2fs_free(&rfs->new_fs);
>  	if (rfs->itable_buf)
>  		ext2fs_free_mem(&rfs->itable_buf);
>  	ext2fs_free_mem(&rfs);
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux