-
Notifications
You must be signed in to change notification settings - Fork 579
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
IcingaDB: Don't sync partially initialised objects #10151
IcingaDB: Don't sync partially initialised objects #10151
Conversation
You can also use only this patch to reproduce it: diff --git a/lib/icinga/downtime.cpp b/lib/icinga/downtime.cpp
index 98a65d417..927e53bbc 100644
--- a/lib/icinga/downtime.cpp
+++ b/lib/icinga/downtime.cpp
@@ -78,6 +78,7 @@ void Downtime::OnAllConfigLoaded()
{
ObjectImpl<Downtime>::OnAllConfigLoaded();
+ Utility::Sleep(60);
if (GetServiceName().IsEmpty())
m_Checkable = Host::GetByName(GetHostName());
else
|
My initial intuition would have been to do a check like this somewhere inside something like icinga2/lib/icingadb/icingadb-objects.cpp Lines 285 to 302 in 8beb0b7
That However, the |
a13f571
to
26f43b0
Compare
Code change looks fine and like it should do the job to me. I didn't try to reproduce/test this myself so far though. So if anyone wants to step in 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is indeed a valid fix for the specific problem described in OP.
But let me make an analogy: Your pipe is leaking (undone objects) at least at one particular point. This PR is just a bucket under that one point, whereas I offer to replace the pipe with one of stainless steel:
We'll keep #10057 open for now and try to fix these kinds of bugs by just using the |
This PR prevents the initial IcingaDB configuration dump from accessing partially initialised objects, which is hard to trigger deliberately, but it is possible as one of our customers is experiencing this problem. When Icinga DB attempts to dump the configuration to Redis, the objects are spliced into smaller chunks with a fixed size (500) and this PR goes through each of these chunks and excludes partially initialised objects before we end up in a nullptr dereference and crash the Icinga 2 process. Should these excluded objects later reach the activation process, they are captured via the
OnActiveChanged
signal and processed inIcingaDB::VersionChangedHandler()
as runtime updates.ref/IP/53428
How To Reproduce
First apply this patch
Build and start Icinga 2 (make sure you don't already have downtimes in
/var/lib/icinga2/icinga2/api
)Now create a downtime via the API
While the API call is blocked, restart Redis and observe