-
-
Notifications
You must be signed in to change notification settings - Fork 187
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.
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 #1951
base: development-v6
Are you sure you want to change the base?
Conversation
Still getting some additional compile errors locally; (warnings probably) so what to check if anything else is related to this fix. |
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 |
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. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
done. |
I see you changed |
So as mentioned before, I'm building with the
Anyway, gcc warns about not NULL-ing the pointer upon successful re-allocation. 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
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 ;) |
Sorry, I haven't been clear in my question:
You propose this change: However, as soon as Is this a Does FTL fully compile with these changes or are there additional changes necessary? Which version of |
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 So looking at it a bit closer, it could be, that gcc 'thinks' that because of the loop, So then do you prefer to |
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. |
I will double check as well again as I rebase and rebuild. |
@oliv3r In case you find no time for doing this (I totally understand!) you could also try checking out the latest |
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 :) |
I'm not exactly sure which branches you mean specifically with "development" and "release". Is it "development" = |
@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 |
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 settingptr_in
to NULL after an successful allocation, we can keep GCC happy and our codebase sane.By submitting this pull request, I confirm the following:
git rebase
)