| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140219233917.GJ9176@birch.djwong.org>
Date: Wed, 19 Feb 2014 15:39:17 -0800
From: "Darrick J. Wong" <darrick.wong@...cle.com>
To: Lukas Czerner <lczerner@...hat.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu
Subject: Re: [PATCH] e2fsprogs: make ext2fs_free() to set the pointer to NULL
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@...hat.com>
> ---
> 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@...r.kernel.org
> 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists