Skip to content

Replace manual memory management with RAII containers #10926

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

Open
aditya-prasad1 wants to merge 3 commits into
base: master
Choose a base branch
from
+14 −13
Open

Replace manual memory management with RAII containers #10926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9e5bc3
Refactor Process::IOThreadProc to use RAII containers
aditya-prasad1 Jul 3, 2026
28c7a4d
Refactor IdoMysqlConnection::Escape to use RAII containers
aditya-prasad1 Jul 3, 2026
ba399c9
Update AUTHORS
aditya-prasad1 Jul 3, 2026
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 AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
Aaron Bishop <erroneous@gmail.com>
Adam Bolte <abolte@systemsaviour.com>
Adam James <atj@pulsewidth.org.uk>
Aditya <imadityaprasad@gmail.com>
akrus <akrus@flygroup.st>
Alan Jenkins <alan.christopher.jenkins@gmail.com>
Alan Litster <alan.litster@twentyci.co.uk>
Expand Down
16 changes: 8 additions & 8 deletions lib/base/process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,10 +597,10 @@ bool Process::GetAdjustPriority() const
void Process::IOThreadProc(int tid)
{
#ifdef _WIN32
HANDLE *handles = nullptr;
HANDLE *fhandles = nullptr;
std::vector<HANDLE> handles;
std::vector<HANDLE> fhandles;
#else /* _WIN32 */
pollfd *pfds = nullptr;
std::vector<pollfd> pfds;
Comment on lines -600 to +603

@julianbrost julianbrost Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.)

@jschmidt-icinga jschmidt-icinga Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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... 🤦

@julianbrost julianbrost Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

#endif /* _WIN32 */
int count = 0;
double now;
Expand All @@ -617,13 +617,13 @@ void Process::IOThreadProc(int tid)

count = 1 + l_Processes[tid].size();
#ifdef _WIN32
handles = reinterpret_cast<HANDLE *>(realloc(handles, sizeof(HANDLE) * count));
fhandles = reinterpret_cast<HANDLE *>(realloc(fhandles, sizeof(HANDLE) * count));
handles.resize(count);
fhandles.resize(count);

fhandles[0] = l_Events[tid];

#else /* _WIN32 */
pfds = reinterpret_cast<pollfd *>(realloc(pfds, sizeof(pollfd) * count));
pfds.resize(count);

pfds[0].fd = l_EventFDs[tid][0];
pfds[0].events = POLLIN;
Expand Down Expand Up @@ -670,9 +670,9 @@ void Process::IOThreadProc(int tid)
timeout *= 1000;

#ifdef _WIN32
DWORD rc = WaitForMultipleObjects(count, fhandles, FALSE, timeout == -1 ? INFINITE : static_cast<DWORD>(timeout));
DWORD rc = WaitForMultipleObjects(count, fhandles.data(), FALSE, timeout == -1 ? INFINITE : static_cast<DWORD>(timeout));
#else /* _WIN32 */
int rc = poll(pfds, count, timeout);
int rc = poll(pfds.data(), count, timeout);

if (rc < 0)
continue;
Expand Down
10 changes: 5 additions & 5 deletions lib/db_ido_mysql/idomysqlconnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,15 +722,15 @@ String IdoMysqlConnection::Escape(const String& s)
String utf8s = Utility::ValidateUTF8(s);

size_t length = utf8s.GetLength();
auto *to = new char[utf8s.GetLength() * 2 + 1];

m_Mysql->real_escape_string(&m_Connection, to, utf8s.CStr(), length);
String to;
to.GetData().resize(length * 2 + 1);

String result = String(to);
size_t escapedLength = m_Mysql->real_escape_string(&m_Connection, to.GetData().data(), utf8s.CStr(), length);

delete [] to;
Comment on lines -729 to -731

@julianbrost julianbrost Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

@jschmidt-icinga jschmidt-icinga Jul 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like we set the SQL mode to a known value without NO_BACKSLASH_ESCAPE here:

icinga2/lib/db_ido_mysql/idomysqlconnection.cpp

Line 427 in 98a71ed

Query("SET SESSION SQL_MODE='NO_AUTO_VALUE_ON_ZERO'");

Otherwise IDO wouldn't function, with or without this change, even if the output was always initialized with '\0'.

@Al2Klimov Al2Klimov Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to.GetData().resize(escapedLength);

return result;
return to;
}

Dictionary::Ptr IdoMysqlConnection::FetchRow(const IdoMysqlResult& result)
Expand Down
Loading