| Message ID | 1392810496-9090-1-git-send-email-lczerner@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 2/19/14, 5:48 AM, 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. Hrmph, sorry for obfuscating. ;) I agree that this seems like a good change, especially to make it more consistent with the NULL assignment that ext2fs_free_mem does. Reviewed-by: Eric Sandeen <sandeen@redhat.com> Thanks Lukas! > 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@redhat.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); > 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); > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> This results in an ABI change, so if there are any userspace programs which are linked against the libext2fs shared library, it would break. So we have two choices: 1) Audit all of the callers of ext2fs_freefs to see if it's necessary to clear the pointer. If the pointer is about to be overwritten, or it's on a stack-allocated variable that will disappear as soon as you return, it's not a problem. Given that your patch had to change every single ext2fs_free() call stack, it's relatively easy to make sure we don't miss any. :-) 2) Define a new ext2fs_freefs2() which takes the changed interface. I'm not entirely convinced that (2) is worth it, but certainly (1) would be a good thing to do. It may be that the problem you couldn't replicate in the latest version of e2fsprogs was one that was fixed either when I did my periodic valgrind test runs, or as a result of reviewing all of the coverity warnings. Cheers, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.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@vger.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 19 Feb 2014, Theodore Ts'o wrote: > Date: Wed, 19 Feb 2014 12:57:14 -0500 > From: Theodore Ts'o <tytso@mit.edu> > To: Lukas Czerner <lczerner@redhat.com> > Cc: linux-ext4@vger.kernel.org > 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@redhat.com> > > This results in an ABI change, so if there are any userspace programs > which are linked against the libext2fs shared library, it would break. > > So we have two choices: > > 1) Audit all of the callers of ext2fs_freefs to see if it's necessary > to clear the pointer. If the pointer is about to be overwritten, or > it's on a stack-allocated variable that will disappear as soon as you > return, it's not a problem. Given that your patch had to change every > single ext2fs_free() call stack, it's relatively easy to make sure we > don't miss any. :-) > > 2) Define a new ext2fs_freefs2() which takes the changed interface. > > I'm not entirely convinced that (2) is worth it, but certainly (1) > would be a good thing to do. I though that having the solution (2) would save us future problems that is why I went this one. But If you really think it's not worth it, I am fine with the (1) as well. I'll send a patch. Thanks! -Lukas > > It may be that the problem you couldn't replicate in the latest > version of e2fsprogs was one that was fixed either when I did my > periodic valgrind test runs, or as a result of reviewing all of the > coverity warnings. > > Cheers, > > - Ted > > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 19 Feb 2014, Darrick J. Wong wrote: > Date: Wed, 19 Feb 2014 15:39:17 -0800 > From: Darrick J. Wong <darrick.wong@oracle.com> > To: Lukas Czerner <lczerner@redhat.com> > Cc: linux-ext4@vger.kernel.org, tytso@mit.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@redhat.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 *)? From the quick look it seems that users of ext2fs_close2 (or ext2fs_close) are doing the right thing when needed and setting the pointer to NULL afterwards. But i will need careful auditing as well. Thanks for pointing that out. > > 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. See my response to Ted, I'd like to avoid ABI change. Thanks! -Lukas > > --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@vger.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); 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);
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@redhat.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(-)