Fix potential nullptr-dereference in perfdata writer stats functions

[jschmidt-icinga]

This fixes a crash in GelfWriter, GraphiteWriter and OpenTsdbWriter that occurs when the writer's stats functions are called, for example by the icinga check plugin, before the writer has been initialized.

Fixes #10842.

[julianbrost]

Is it even safe to access that member in that way here? If that pointer can chance concurrently, there should probably be some locking protecting the access.

[jschmidt-icinga]

I've changed the queries to be guarded with !obj->IsPaused() instead. I think this should be safe because once the object is no longer paused, it will always have a connection object.

But it occurs to me that this might need an additional ObjectLock, since in the unlikely event that the thread that gathers the stats is preempted after IsPaused(), but before m_Connection is dereferenced, and we then have an entire Pause/Resume cycle, there could still be a race.

[julianbrost]

I've changed the queries to be guarded with !obj->IsPaused() instead. I think this should be safe because once the object is no longer paused, it will always have a connection object.

Doesn't this rely on how the current implementation only initializes m_Connection in Resume(), but doesn't reset it in Pause()? That's somewhat inconsistent and doesn't sound like something I'd want to rely on (it's just an "remove inconsistency" fix away from bringing back an issue).

The PerfdataWriterConnection constructor doesn't already establish a connection by itself, does it? So one option could be to initialize the connection earlier, so that it is guaranteed to exist without any additional check (though that would need another look to ensure that there really isn't yet another race condition).

In Icinga DB, there's a pretty similar situation. Have a look at how it's done there (short version: it stores two copies of the pointer, one for fast internal access without explicit locking (Icinga DB makes sure that version is only ever accessed from one thread/strand) and one for external access with locking):

icinga2/lib/icingadb/icingadb.hpp

Lines 446 to 450 in 6c73bb4

	RedisConnection::Ptr m_Rcon;
	// m_RconLocked contains a copy of the value in m_Rcon where all accesses are guarded by a mutex to
	// allow safe concurrent access like from the icingadb check command. It's a copy to still allow fast access
	// without additional synchronization to m_Rcon within the IcingaDB feature itself.
	Locked<RedisConnection::Ptr> m_RconLocked;