On Thu, Mar 27, 2025 at 04:51:46PM +0530, shivam tiwari wrote:
> Dear Wget Developers,
>
> This patch fixes a Heap-use-after-free vulnerability in the
> hash_table_destroy function.
As a side note, I would argue that it actually doesn't... What it
does is safeguard against other parts of the code which have a use
after free bug calling it. The distinction is perhaps subtle, but I
think important.
In other words, if there is code which is calling hash_table_destroy
on a struct hash_table pointer that has already been freed, THAT is a
bug, and that should be fixed. Your patch (and my suggested variants
of it) actually just masks (or delays) the real problem, IMO.
For whatever that's worth...
On Mon, Apr 21, 2025 at 11:42:17AM -0400, Derek Martin wrote:
> On Thu, Mar 27, 2025 at 04:51:46PM +0530, shivam tiwari wrote:
> > Dear Wget Developers,
> >
> > This patch fixes a Heap-use-after-free vulnerability in the
> > hash_table_destroy function.
>
> Note:
>
> diff --git a/src/hash.c b/src/hash.c
> index 670ec1cf..00e07eef 100644
> --- a/src/hash.c
> +++ b/src/hash.c
> @@ -305,8 +305,13 @@ hash_table_new (int items,
> void
> hash_table_destroy (struct hash_table *ht)
> {
> - xfree (ht->cells);
> - xfree (ht);
> + if (ht)
> + {
> + xfree (ht->cells);
> + ht->cells = NULL;
> + xfree (ht);
> + ht = NULL;
> + }
> }
>
> The line:
>
> ht = NULL;
>
> is ineffectual. In this function, ht is a local (function scope) copy
> of the pointer. For the intended thing to happen, you need this:
>
> void
> hash_table_destroy(struct hash_table **ht)
> {
> if (ht && *ht)
> {
> xfree(*ht->cells);
> *ht->cells = NULL;
> xfree(*ht);
> *ht = NULL;
> }
> }
>
> And then everywhere it's called, you need to change:
>
> struct hash_table *foo;
> ...
> hash_table_destroy(foo);
>
> to:
>
> hash_table_destroy(&foo);
>
> Alternatively, you could remove the assignment, and use a macro
> similar to:
>
> #define hash_table_free(htp) do { hash_table destroy(htp); htp = NULL; }
> while (0);
>
> (The solution with the least code changes would be to rename the
> function and give the macro its original name...)
>
>
> And for the sake of completeness, a short example to demonstrate this:
>
> $ cat free.c
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
>
> struct hash_table {
> void *cells;
> };
>
> void hash_table_destroy(struct hash_table *ht)
> {
> if (ht)
> {
> free(ht->cells);
> /* don't need to check ht->cells, free(NULL) is a no-op on all
> non-broken systems */
> ht->cells = NULL;
> free(ht);
> ht = NULL;
> }
> }
>
> int main(int argc, char **argv)
> {
> struct hash_table *foo;
> foo = (struct hash_table *)malloc(sizeof *foo);
> foo->cells = NULL;
> printf("Address of *foo is %p\n", foo);
> hash_table_destroy(foo);
> printf("Address of *foo is %p\n", foo);
> return 0;
> }
>
> $ gcc -o free free.c
> $ ./free
> Address of *foo is 0x6243cfb262a0
> Address of *foo is 0x6243cfb262a0
>
>
>
--
Derek Martin
Principal System Software Engineer
Akamai Technologies
demar...@akamai.com
