Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 18 Jan 2023 00:25:54 -0500
From:   "Theodore Ts'o" <tytso@mit.edu>
To:     zhanchengbin <zhanchengbin1@huawei.com>
Cc:     linux-ext4@vger.kernel.org, liuzhiqiang26@huawei.com,
        linfeilong@huawei.com, riteshh <riteshh@linux.ibm.com>
Subject: Re: [PATCH v3] setup_tdb : fix memory leak
Message-ID: <Y8eC4uM9b3hiBpfR@mit.edu>
References: <da92f94e-14ff-daaa-57e5-43e91bd9ff4c@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <da92f94e-14ff-daaa-57e5-43e91bd9ff4c@huawei.com>
Precedence: bulk

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