Avoid use-after-free warning

[oliv3r]

More recent versions of GCC try to check for use after free. Realloc will take the input pointer, ptrn and with free it after a successful allocation. However, it will not change the contents of the pointer, and thus GCC in some cases thinks it can/could be used. By explicitly setting ptrn to NULL after an successful allocation, we can keep GCC happy and our codebase sane.

[dag-erling]

Can you provide additional details about the environment in which you experience the use-after-free warning? Operating system, compiler version, configure options?

[dag-erling]

Here, we attempt to allocate a new object with the new size. If successful, the old object will be freed.

[dag-erling]

Next, we check if we were successful.

As a side note, we could save ourselves a bit of work if we checked not only that new_ptr is not NULL but also that it is not equal to ptr, which can happen, especially if new_size is either smaller than the original size, or only very slightly larger.

[dag-erling]

If the reallocation was successful, we need to remove the old pointer from the hash table, then add the new one.

However, setting ptr to NULL before calling hash_table_del() prevents removing the old value from the hash table, so we risk attempting to access or free it later. While trying to silence a bogus use-after-free warning, you have in fact introduced a real use-after-free bug.

[oliv3r]

I see the error of my ways :) but does hash_table_del set ptr to null?

So we reallocated to new_ptr and the old pointer is now no longer valid (assuming they are not identical, as you rightfully point out, that this could happen, unless you'd check the size before allocation). realloc thus states,

The realloc() function returns a pointer to the newly allocated memory, which is suitably aligned for any kind of variable and may be different from ptr, or NULL if the request fails.

Sure, I was trying to build things on alpine:latest; which comes with gcc (Alpine 13.2.1_git20240309) 13.2.1 20240309

Here's the compiler warning, which causes trouble when building with -Werror. Note that this is from a downstream project, pihole FTL dns, which copies these files from here.

/workdir/src/FTL-5.25.2/src/tre-regex/xmalloc.c: In function 'xrealloc_impl':
/workdir/src/FTL-5.25.2/src/tre-regex/xmalloc.c:343:7: warning: pointer 'ptr' may be used after 'realloc' [-Wuse-after-free]
  343 |       hash_table_del(xmalloc_table, ptr);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/workdir/src/FTL-5.25.2/src/tre-regex/xmalloc.c:340:13: note: call to 'realloc' here
  340 |   new_ptr = realloc(ptr, new_size);
      |             ^~~~~~~~~~~~~~~~~~~~~~
[ 47%] Built target tre-regex

So anyway, while you are right, gcc doesnt' understand/spot the use of the hash-table, which needs to old ptr.

Anyway, we want to keep -Wuse-after-free (or -Wall or whatever we have available). We also want to be able to do -Werror. I understand that you know what you are doing and this is expected, but a comment and a note to gcc that this is okay would then be needed.

Also, after hash_table_del is done with the old pointer, we should still assigned NULL to the ptr no?

[dag-erling]

1. Checking the size before calling realloc() serves no purpose. We have no way of knowing what realloc() will do until it returns. It might be able to adjust the existing allocation without moving it. It might also decide not to do so even if it could, to reduce heap fragmentation, for instance.
2. Setting ptr to NULL serves no purpose if it's going out of scope anyway. It won't make the warning go away.
3. The warning occurs because gcc, when compiling this code, does not know that hash_table_del() does not dereference ptr. This can be fixed by annotating hash_table_del() with __attribute__((access(none, 2))), but care must be taken not to break support for compilers that don't understand gcc's __attribute__ syntax or don't support the access attribute.

[dag-erling]

Can you please check if #107 fixes your issue?

[dag-erling]

Obsoleted by #107.