On Sat, Dec 13, 2014 at 08:13:27PM -0500, Theodore Ts'o wrote:
> On Fri, Nov 07, 2014 at 01:51:59PM -0800, Darrick J. Wong wrote:
> > + if (inode->i_flags & EXT4_EXTENTS_FL) {
> > + err = ext2fs_extent_open2(fs, ino, inode, &handle);
> > + if (err)
> > + goto no_blocks;
> > + err = ext2fs_extent_goto2(handle, 0, lblk);
> > + if (err)
> > + goto extent_err;
> > + err = ext2fs_extent_get(handle, EXT2_EXTENT_CURRENT, &extent);
> > + if (err)
> > + goto extent_err;
> > + return extent.e_pblk + (lblk - extent.e_lblk);
>
> There's a memory leak here; the extent handle never gets freed. I've
> checked this in with the attached fix up patched.
>
> BTW, a really useful way of checking for memory leaks and which
> allowed me to confirm the problem after noticing the problem from
> reviewing the patch (and showing that with the attached patch below
> the leak went away):
>
> cd tests; ./test_script --valgrind-leakcheck f_extents f_extents2
> more /tmp/valgrind-*
Oops, thank you for catching this one. I periodically run make check with
USE_VALGRIND, forgot to do so. :/
--D
>
> Cheers,
>
> - Ted
>
> diff --git a/lib/ext2fs/alloc.c b/lib/ext2fs/alloc.c
> index 57bf5f0..62b36fe 100644
> --- a/lib/ext2fs/alloc.c
> +++ b/lib/ext2fs/alloc.c
> @@ -296,7 +296,7 @@ blk64_t ext2fs_find_inode_goal(ext2_filsys fs, ext2_ino_t ino,
> dgrp_t group;
> __u8 log_flex;
> struct ext2fs_extent extent;
> - ext2_extent_handle_t handle;
> + ext2_extent_handle_t handle = NULL;
> errcode_t err;
>
> if (inode == NULL || ext2fs_inode_data_blocks2(fs, inode) == 0)
> @@ -311,14 +311,12 @@ blk64_t ext2fs_find_inode_goal(ext2_filsys fs, ext2_ino_t ino,
> goto no_blocks;
> err = ext2fs_extent_goto2(handle, 0, lblk);
> if (err)
> - goto extent_err;
> + goto no_blocks;
> err = ext2fs_extent_get(handle, EXT2_EXTENT_CURRENT, &extent);
> if (err)
> - goto extent_err;
> - return extent.e_pblk + (lblk - extent.e_lblk);
> -extent_err:
> + goto no_blocks;
> ext2fs_extent_free(handle);
> - goto no_blocks;
> + return extent.e_pblk + (lblk - extent.e_lblk);
> }
>
> /* block mapped file; see if block zero is mapped? */
> @@ -326,6 +324,7 @@ extent_err:
> return inode->i_block[0];
>
> no_blocks:
> + ext2fs_extent_free(handle);
> log_flex = fs->super->s_log_groups_per_flex;
> group = ext2fs_group_of_ino(fs, ino);
> if (log_flex)
--
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
