Avoid use-after-free warning

[oliv3r]

More recent versions of GCC try to check for use after free. Realloc will take the input pointer, ptr_in 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 ptr_in to NULL after an successful allocation, we can keep GCC happy and our codebase sane.

In file included from /workdir/src/FTL-5.25.2/src/syscalls/realloc.c:13:
/workdir/src/FTL-5.25.2/src/syscalls/realloc.c: In function 'FTLrealloc':
/workdir/src/FTL-5.25.2/src/syscalls/../log.h:30:27: error: pointer 'ptr_in' may be used after 'realloc' [-Werror=use-after-free]
   30 | #define logg(format, ...) _FTL_log(true, false, format, ## __VA_ARGS__)
      |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/workdir/src/FTL-5.25.2/src/syscalls/realloc.c:42:17: note: in expansion of macro 'logg'
   42 |                 logg("FATAL: Memory reallocation (%p -> %zu) failed in %s() (%s:%i)",
      |                 ^~~~
/workdir/src/FTL-5.25.2/src/syscalls/realloc.c:31:27: note: call to 'realloc' here
   31 |                 ptr_out = realloc(ptr_in, size);
      |                           ^~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

---

By submitting this pull request, I confirm the following:

1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
2. I have commented my proposed changes within the code and I have tested my changes.
3. I am willing to help maintain this change if there are issues with it later.
4. It is compatible with the EUPL 1.2 license
5. I have squashed any insignificant commits. (git rebase)
6. I have checked that another pull request for this purpose does not exist.
7. I have considered, and confirmed that this submission will be valuable to others.
8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
9. I give this submission freely, and claim no ownership to its content.

---

• [x] I have read the above and my PR is ready for review. Check this box to confirm

[oliv3r]

Still getting some additional compile errors locally; (warnings probably) so what to check if anything else is related to this fix. error: inlining failed in call to 'gravityDB_finalize_client_statements': --param max-inline-insns-single limit reached [-Werror=inline]

[DL6ER]

Thank you for your submission. However, note that we are currently in a public beta of Pi-hole v6.0 and this issue seems to ring a bell. Please check if the issue exists in the branch development-v6 as well and, if so, please file your pull request against this branch which will soon(ish) replace the code in branch master

[oliv3r]

Thank you for your submission. However, note that we are currently in a public beta of Pi-hole v6.0 and this issue seems to ring a bell. Please check if the issue exists in the branch development-v6 as well and, if so, please file your pull request against this branch which will soon(ish) replace the code in branch master

done.

[DL6ER]

I see you changed FTLrealloc() but where could ptr_in be reused after a successful call to realloc() (ptr_out != NULL)? This is the justification for your change and I'm not seeing it TBH. Also I'm not very happy with the change in the tre-regex directory as this is external code we "simply" inlined into our code. Any future updates of this library will remove this if we forget about the manual change (we most likely will...). I'm still okay with this one here because maintenance on tre-regex is basically non-existent and it's unclear when (if at all) there will be a new version.

[oliv3r]

I see you changed FTLrealloc() but where could ptr_in be reused after a successful call to realloc() (ptr_out != NULL)? This is the justification for your change and I'm not seeing it TBH.

So as mentioned before, I'm building with the Release target which probably enables -Werror? TBF I see still warnings such as:

FTL-5.25.2/src/tre-regex/xmalloc.h:69:20: warning: 'free' called on pointer to an unallocated object [-Wfree-nonheap-object]
   69 | #define xfree(ptr) free(ptr)
      |                    ^~~~~~~~~
/workdir/src/FTL-5.25.2/src/tre-regex/tre-match-backtrack.c:140:17: note: in expansion of macro 'xfree'
  140 |                 xfree(tags);                                                  \
      |                 ^~~~~
/workdir/src/FTL-5.25.2/src/tre-regex/tre-match-backtrack.c:353:11: note: in expansion of macro 'BT_STACK_PUSH'
  353 |           BT_STACK_PUSH(pos, str_byte, str_wide, trans_i->state,
      |           ^~~~~~~~~~~~~
In file included from /usr/include/stdlib.h:140,
                 from /usr/include/fortify/stdlib.h:23,
                 from /workdir/src/FTL-5.25.2/src/tre-regex/tre-match-backtrack.c:58:
/workdir/src/FTL-5.25.2/src/tre-regex/tre-match-backtrack.c:272:10: note: returned from '__builtin_alloca'
  272 |   tags = alloca(sizeof(*tags) * tnfa->num_tags);
      |          ^

Anyway, gcc warns about not NULL-ing the pointer upon successful re-allocation. realloc() does free(ptr_in) internally on success, they should also have ptr_in = NULL internally done as well. So gcc has some easy checks to see if ptr_in is NULL or not. So Not NULL-ing it, means we can not compile the code anymore with the error mentioned in the first post.

So the question shouldn't be 'but where could it possibly happen'. So either we do this change to make gcc happy, or disable the warning -Werror=use-after-free, but I generally am not a fan of doing that, not even if you could tell gcc to ignore the warning on that specific line.

Also I'm not very happy with the change in the tre-regex directory as this is external code we "simply" inlined into our code. Any future updates of this library will remove this if we forget about the manual change (we most likely will...). I'm still okay with this one here because maintenance on tre-regex is basically non-existent and it's unclear when (if at all) there will be a new version.

I think we should keep it here of course, for the same reason as above. I don't mind opening a merge request upstream as well ;)

https://github.com/laurikari/tre/pull/100

[DL6ER]

Sorry, I haven't been clear in my question:

I see you changed FTLrealloc() but where could ptr_in be reused after a successful call to realloc() (ptr_out != NULL)? This is the justification for your change and I'm not seeing it TBH.

You propose this change:

However, as soon as realloc() returns successfully, we have ptr_out != NULL and the do { } while() condition is violated. ptr_in is then never accessed again. Don't get me wrong, I do see the relevance in the tre part, but I don't see it here.

Is this a gcc bug you are circumventing here where it seems to see such reusage even if there is none in reality?

Does FTL fully compile with these changes or are there additional changes necessary? Which version of gcc are you using exactly? 14.1?

[oliv3r]

Sorry, I haven't been clear in my question:

I see you changed FTLrealloc() but where could ptr_in be reused after a successful call to realloc() (ptr_out != NULL)? This is the justification for your change and I'm not seeing it TBH.

You propose this change:

However, as soon as `realloc()` returns successfully, we have `ptr_out != NULL` and the `do { } while()` condition is violated. `ptr_in` is then never accessed again. Don't get me wrong, I **do** see the relevance in the `tre` part, but I don't see it here. Is this a `gcc` bug you are circumventing here where it seems to see such reusage even if there is none in reality? Does FTL fully compile with these changes or are there additional changes necessary? Which version of `gcc` are you using exactly? 14.1?

Ah, this is a fair point indeed, however, without this change, I am unable to compile things because it is treated as error, which brought me to this PR in the first place :) But not sure if's a 'bug', more of a how things are used. Because realloc is being done in a 'loop', gcc probably misses/misunderstands things, however gcc does expect ptr_in to be NULLed. I first had it outside of the loop, but gcc thought it was smarter.

As for the gcc version, should be 13.2.1 20231014

So looking at it a bit closer, it could be, that gcc 'thinks' that because of the loop, ptr_in is 'always' valid/in-use. So while one could argue weather its best practice to always NULL after a successful reallocation, gcc certainly seems to think so/is blind to the fact that ptr_in is not used any further.

So then do you prefer to silence gcc for this specific bit 'because gcc is being not smart enough` (kind of ugly), or just follow the suggested best practice, of always NULLing after successful allocation. I suppose it's a matter of 'pick your poison' in that case.

[DL6ER]

It's a clear choice, we never want to silence warnings, who knows what future GCC versions would show us, otherwise. I just want to understand why we need this because it didn't seem useful to me.

I'll review this change once back home.

[DL6ER]

I did just upgrade my local gcc and seemingly don't need the questionable change! I added these three changes and FTL compiles without a single warning on gcc 13.2.1:

• https://github.com/pi-hole/FTL/commit/fccbe2bee4c14c4d1b0a6f19d6a91c02bec76922
• https://github.com/pi-hole/FTL/commit/55258150c1e35e19b35d6d39153ba6e4a0cb92bb
• https://github.com/pi-hole/FTL/commit/91b1ced3ab1e2b5ad02268262b35d7238a5b3dc5

[oliv3r]

I will double check as well again as I rebase and rebuild.

[DL6ER]

@oliv3r In case you find no time for doing this (I totally understand!) you could also try checking out the latest special/CI_development branch. This one is meant to compile fine with gcc 13.2.1 (the current compiler in alpine:v3.19alpha)

[oliv3r]

@oliv3r In case you find no time for doing this (I totally understand!) you could also try checking out the latest special/CI_development branch. This one is meant to compile fine with gcc 13.2.1 (the current compiler in alpine:v3.19alpha)

So the issue goes away on the development branch, but on the release branch we still need it.

So the question then is; what is different between develop and the release branch in this context. Build flags is the only thing that I can come up with; because the compiler is the same.

Anyway, with the patch(es) it builds fine; see here: https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/66136

So once you release v6 I'll drop the patches :)

[DL6ER]

I'm not exactly sure which branches you mean specifically with "development" and "release". Is it "development" = development-v6 and "release" = master? If so, rest assured that development-v6 is the current one and will become/replace master soon(ish).

[oliv3r]

@DL6ER I apologize, release branch is indeed the master branch/previously tagged branches. I have put this issue on the back-burner a bit for myself, and will dig into it again once alpine gets some things sorted :) as it's making life much harder then I want it to be :p