Replace manual memory management with RAII containers#10926
Replace manual memory management with RAII containers#10926aditya-prasad1 wants to merge 3 commits into
Conversation
|
Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA). Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA. After that, please reply here with a comment and we'll verify. Contributors that have not signed yet: @aditya-prasad1 Details
|
|
I've signed the CLA |
|
@cla-bot check |
There was a problem hiding this comment.
In case you're unaware what the failed AUTHORS check means, you need to add yourself to the AUTHORS file. You can add a second commit to this PR, it doesn't have to be a separate PR.
PS: Never mind the failed debian:11 check. It does that sometimes, but it will likely go away on your next push and if it doesn't, you can just restart it (or let us worry about that).
PPS: Ideally make the change to lib/base/process.cpp and lib/db_ido_mysql/idomysqlconnection.cpp separate commits. Wouldn't block a merge in my eyes, but would be preferable.
There was a problem hiding this comment.
Aside from the whitespace errors below this looks good to me now.
When you're done with that, please squash down or reformat your commits so you have exactly three: One for process.cpp, one for idomysqlconnection.cpp and one for AUTHORS.
PS: Also maybe label the AUTHORS commit something like Add Aditya to AUTHORS file or just Update AUTHORS file to avoid first person.
aditya-prasad1
force-pushed
the
fix-raii-10813
branch
2 times, most recently
from
361b17d to
829492c
Compare
aditya-prasad1
force-pushed
the
fix-raii-10813
branch
from
829492c to
9fef56d
Compare
|
@jschmidt-icinga Can you approve the CI check workflow? |
jschmidt-icinga
force-pushed
the
fix-raii-10813
branch
from
9fef56d to
ba399c9
Compare
jschmidt-icinga
left a comment
jschmidt-icinga
left a comment
There was a problem hiding this comment.
I just quickly fixed the whitespace myself, but now it looks ready to merge. Thank you for your contribution.
| HANDLE *handles = nullptr; | ||
| HANDLE *fhandles = nullptr; | ||
| std::vector<HANDLE> handles; | ||
| std::vector<HANDLE> fhandles; | ||
| #else /* _WIN32 */ | ||
| pollfd *pfds = nullptr; | ||
| std::vector<pollfd> pfds; |
There was a problem hiding this comment.
I'm surprised that this PR does not remove any free() calls, but there aren't any for these. Does that mean that this fixes a memory leak (technically, it should only leak the allocation when exiting the process, so not something you would really notice)? Or asking differently: did someone check that these free() calls were just missing (there would be a possibility that there was an API that wants a malloc()-allocated pointer and takes ownership of it, at first glance, this doesn't look like this here, but better double-check it)?
(This is just a question for the moment, request changes is for the other comment.)
There was a problem hiding this comment.
I think this was relying on Process::IOThreadProc() running for the entire program duration (the for(;;) loop is never escaped) and only ever using realloc() to allocate and free previous allocations. So yes, this had a memory leak, but it's only ever the last allocation that never gets deallocated when the program shuts down.
This doesn't really fix that though, since the for loop is still never escaped so the RAII doesn't really do anything here. Also this means the threads are never joined, which probably isn't ideal either... 🤦
There was a problem hiding this comment.
I didn't want to put a focus on fixing a more or less theoretical memory leak here, just on making sure that the free() wasn't missing intentionally.
| String result = String(to); | ||
| size_t escapedLength = m_Mysql->real_escape_string(&m_Connection, to.GetData().data(), utf8s.CStr(), length); | ||
|
|
||
| delete [] to; |
There was a problem hiding this comment.
mysql_real_escape_string() may return (unsigned long)-1 to signal errors which would not be handled correctly now (I don't really know if it was before, didn't read anything on whether it is guaranteed to write '\0' into to on errors).
There was a problem hiding this comment.
It looks like we set the SQL mode to a known value without NO_BACKSLASH_ESCAPE here:
Otherwise IDO wouldn't function, with or without this change, even if the output was always initialized with '\0'.
There was a problem hiding this comment.
- In doubt, we can leave the IDO untouched, given Remove deprecated features #10731.
5 participants
Replaced manual temporary buffer management with
std::vectorin:Process::IOThreadProc()IdoMysqlConnection::Escape()This removes explicit allocation/deallocation and relies on RAII while preserving existing behavior.
Closes #10813.