Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid use-after-free warning #100

Closed
wants to merge 1 commit into from
Closed

Avoid use-after-free warning #100

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/xmalloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,7 @@ xrealloc_impl(void *ptr, size_t new_size, const char *file, int line,
new_ptr = realloc(ptr, new_size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (new_ptr != NULL)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
ptr = NULL;
hash_table_del(xmalloc_table, ptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

@dag-erling dag-erling Jun 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

hash_table_add(xmalloc_table, new_ptr, (int)new_size, file, line, func);
}
Expand Down