diff mbox series

[v3] setup_tdb : fix memory leak

Message ID da92f94e-14ff-daaa-57e5-43e91bd9ff4c@huawei.com
State Rejected
Headers show
Series [v3] setup_tdb : fix memory leak | expand

Commit Message

zhanchengbin Jan. 4, 2022, 12:09 p.m. UTC 
In setup_tdb(), need free tdb_dir and db->tdb_fn before return,
otherwise it will cause memory leak.

Signed-off-by: zhanchengbin <zhanchengbin1@huawei.com>
Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com>
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
  e2fsck/dirinfo.c | 17 +++++++++++++----
  1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Theodore Ts'o Jan. 18, 2023, 5:25 a.m. UTC | #1 
On Tue, Jan 04, 2022 at 08:09:16PM +0800, zhanchengbin wrote:
> In setup_tdb(), need free tdb_dir and db->tdb_fn before return,
> otherwise it will cause memory leak.

This patch is not correct.  tdb_dir is returned by
profile_get_string(), and it does *not* need to be freed.  In fact, if
you had tested this using valgrind or AddressSanitizer, it would have
failed because tdb_dir is in the *middle* of an allocated block[1],
and this would have corrupted the heap data structures leading to all
sorts of potential problems.

[1] And that allocated block is freed by profile_release()

In addition, tdb->tdb_fn will be freed by e2fsck_free_dir_info(), and
so freeing it on the error path in setup_tdb() will result in a
double-free when e2fsck_free_dir_info() gets called.

I'm guessing you didn't actually test this patch with the code paths
in question --- that is, by triggering an error while using something
like ASan or valgrid?  Note that corrupting the heap may lead to an
exploitable security bug, so if you have applied this patch in your
production version of e2fsprogs, I suggest that you revert it.

Cheers,

					- Ted
diff mbox series

Patch

diff --git a/e2fsck/dirinfo.c b/e2fsck/dirinfo.c
index 49d624c5..5b1088d4 100644
--- a/e2fsck/dirinfo.c
+++ b/e2fsck/dirinfo.c
@@ -49,7 +49,7 @@  static void setup_tdb(e2fsck_t ctx, ext2_ino_t num_dirs)
  	ext2_ino_t		threshold;
  	errcode_t		retval;
  	mode_t			save_umask;
-	char			*tdb_dir, uuid[40];
+	char			*tdb_dir = NULL, uuid[40];
  	int			fd, enable;

  	profile_get_string(ctx->profile, "scratch_files", "directory", 0, 0,
@@ -61,11 +61,11 @@  static void setup_tdb(e2fsck_t ctx, ext2_ino_t num_dirs)

  	if (!enable || !tdb_dir || access(tdb_dir, W_OK) ||
  	    (threshold && num_dirs <= threshold))
-		return;
+		goto tdb_dir_error;

  	retval = ext2fs_get_mem(strlen(tdb_dir) + 64, &db->tdb_fn);
  	if (retval)
-		return;
+		goto tdb_dir_error;

  	uuid_unparse(ctx->fs->super->s_uuid, uuid);
  	sprintf(db->tdb_fn, "%s/%s-dirinfo-XXXXXX", tdb_dir, uuid);
@@ -74,7 +74,7 @@  static void setup_tdb(e2fsck_t ctx, ext2_ino_t num_dirs)
  	umask(save_umask);
  	if (fd < 0) {
  		db->tdb = NULL;
-		return;
+		goto tdb_fn_error;
  	}

  	if (num_dirs < 99991)
@@ -83,6 +83,15 @@  static void setup_tdb(e2fsck_t ctx, ext2_ino_t num_dirs)
  	db->tdb = tdb_open(db->tdb_fn, num_dirs, TDB_NOLOCK | TDB_NOSYNC,
  			   O_RDWR | O_CREAT | O_TRUNC, 0600);
  	close(fd);
+
+	return;
+
+tdb_fn_error:
+	ext2fs_free_mem(&db->tdb_fn);
+tdb_dir_error:
+	if (tdb_dir) {
+		free(tdb_dir);	
+	}
  }
  #endif