`IfwApiCheckTask` uses thread-local variable in coroutine

[julianbrost]

Random "that can't possibly work" observation while looking for something different:

ReportIfwCheckResult() in ifwapichecktask.cpp uses Checkable::ExecuteCommandProcessFinishedHandler:

icinga2/lib/methods/ifwapichecktask.cpp

Lines 36 to 49 in 4c6b93d

	static void ReportIfwCheckResult(
	const Checkable::Ptr& checkable, const Value& cmdLine, const CheckResult::Ptr& cr,
	const String& output, double start, double end, int exitcode = 3, const Array::Ptr& perfdata = nullptr
	)
	{
	if (Checkable::ExecuteCommandProcessFinishedHandler) {
	ProcessResult pr;
	pr.PID = -1;
	pr.Output = perfdata ? output + " |" + String(perfdata->Join(" ")) : output;
	pr.ExecutionStart = start;
	pr.ExecutionEnd = end;
	pr.ExitStatus = exitcode;
	Checkable::ExecuteCommandProcessFinishedHandler(cmdLine, pr);

However, Checkable::ExecuteCommandProcessFinishedHandler is defined as thread_local:

icinga2/lib/icinga/checkable.cpp

Line 34 in 4c6b93d

thread_local std::function<void(const Value& commandLine, const ProcessResult&)> Checkable::ExecuteCommandProcessFinishedHandler;

And ReportIfwCheckResult() is used from within DoIfwNetIo():

icinga2/lib/methods/ifwapichecktask.cpp

Lines 96 to 284 in 4c6b93d

	static void DoIfwNetIo(
	boost::asio::yield_context yc, const Checkable::Ptr& checkable, const Array::Ptr& cmdLine,
	const CheckResult::Ptr& cr, const String& psCommand, const String& psHost, const String& san, const String& psPort,
	AsioTlsStream& conn, boost::beast::http::request<boost::beast::http::string_body>& req, double start
	)
	{
	namespace http = boost::beast::http;
	boost::beast::flat_buffer buf;
	http::response<http::string_body> resp;
	try {
	Connect(conn.lowest_layer(), psHost, psPort, yc);
	} catch (const std::exception& ex) {
	ReportIfwCheckResult(
	yc, checkable, cmdLine, cr,
	"Can't connect to IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
	start
	);
	return;
	}
	auto& sslConn (conn.next_layer());
	try {
	sslConn.async_handshake(conn.next_layer().client, yc);
	} catch (const std::exception& ex) {
	ReportIfwCheckResult(
	yc, checkable, cmdLine, cr,
	"TLS handshake with IfW API on host '" + psHost + "' (SNI: '" + san
	+ "') port '" + psPort + "' failed: " + GetUnderstandableError(ex),
	start
	);
	return;
	}
	if (!sslConn.IsVerifyOK()) {
	auto cert (sslConn.GetPeerCertificate());
	Value cn;
	try {
	cn = GetCertificateCN(cert);
	} catch (const std::exception&) {
	}
	ReportIfwCheckResult(
	yc, checkable, cmdLine, cr,
	"Certificate validation failed for IfW API on host '" + psHost + "' (SNI: '" + san + "'; CN: "
	+ (cn.IsString() ? "'" + cn + "'" : "N/A") + ") port '" + psPort + "': " + sslConn.GetVerifyError(),
	start
	);
	return;
	}
	try {
	http::async_write(conn, req, yc);
	conn.async_flush(yc);
	} catch (const std::exception& ex) {
	ReportIfwCheckResult(
	yc, checkable, cmdLine, cr,
	"Can't send HTTP request to IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
	start
	);
	return;
	}
	try {
	http::async_read(conn, buf, resp, yc);
	} catch (const std::exception& ex) {
	ReportIfwCheckResult(
	yc, checkable, cmdLine, cr,
	"Can't read HTTP response from IfW API on host '" + psHost + "' port '" + psPort + "': " + GetUnderstandableError(ex),
	start
	);
	return;
	}
	double end = Utility::GetTime();
	{
	boost::system::error_code ec;
	sslConn.async_shutdown(yc[ec]);
	}
	CpuBoundWork cbw (yc);
	Value jsonRoot;
	try {
	jsonRoot = JsonDecode(resp.body());
	} catch (const std::exception& ex) {
	ReportIfwCheckResult(
	checkable, cmdLine, cr,
	"Got bad JSON from IfW API on host '" + psHost + "' port '" + psPort + "': " + ex.what(), start, end
	);
	return;
	}
	if (!jsonRoot.IsObjectType<Dictionary>()) {
	ReportIfwCheckResult(
	checkable, cmdLine, cr,
	"Got JSON, but not an object, from IfW API on host '"
	+ psHost + "' port '" + psPort + "': " + JsonEncode(jsonRoot),
	start, end
	);
	return;
	}
	Value jsonBranch;
	if (!Dictionary::Ptr(jsonRoot)->Get(psCommand, &jsonBranch)) {
	ReportIfwCheckResult(
	checkable, cmdLine, cr,
	"Missing ." + psCommand + " in JSON object from IfW API on host '"
	+ psHost + "' port '" + psPort + "': " + JsonEncode(jsonRoot),
	start, end
	);
	return;
	}
	if (!jsonBranch.IsObjectType<Dictionary>()) {
	ReportIfwCheckResult(
	checkable, cmdLine, cr,
	"." + psCommand + " in JSON from IfW API on host '"
	+ psHost + "' port '" + psPort + "' is not an object: " + JsonEncode(jsonBranch),
	start, end
	);
	return;
	}
	Dictionary::Ptr result = jsonBranch;
	Value exitcode;
	if (!result->Get("exitcode", &exitcode)) {
	ReportIfwCheckResult(
	checkable, cmdLine, cr,
	"Missing ." + psCommand + ".exitcode in JSON object from IfW API on host '"
	+ psHost + "' port '" + psPort + "': " + JsonEncode(result),
	start, end
	);
	return;
	}
	static const std::set<double> exitcodes {ServiceOK, ServiceWarning, ServiceCritical, ServiceUnknown};
	static const auto exitcodeList (Array::FromSet(exitcodes)->Join(", "));
	if (!exitcode.IsNumber() || exitcodes.find(exitcode) == exitcodes.end()) {
	ReportIfwCheckResult(
	checkable, cmdLine, cr,
	"Got bad exitcode " + JsonEncode(exitcode) + " from IfW API on host '" + psHost + "' port '" + psPort
	+ "', expected one of: " + exitcodeList,
	start, end
	);
	return;
	}
	auto perfdataVal (result->Get("perfdata"));
	Array::Ptr perfdata;
	try {
	perfdata = perfdataVal;
	} catch (const std::exception&) {
	ReportIfwCheckResult(
	checkable, cmdLine, cr,
	"Got bad perfdata " + JsonEncode(perfdataVal) + " from IfW API on host '"
	+ psHost + "' port '" + psPort + "', expected an array",
	start, end
	);
	return;
	}
	if (perfdata) {
	ObjectLock oLock (perfdata);
	for (auto& pv : perfdata) {
	if (!pv.IsString()) {
	ReportIfwCheckResult(
	checkable, cmdLine, cr,
	"Got bad perfdata value " + JsonEncode(perfdata) + " from IfW API on host '"
	+ psHost + "' port '" + psPort + "', expected an array of strings",
	start, end
	);
	return;
	}
	}
	}
	ReportIfwCheckResult(checkable, cmdLine, cr, result->Get("checkresult"), start, end, exitcode, perfdata);
	}

Which in turn is started as a coroutine:

icinga2/lib/methods/ifwapichecktask.cpp

Lines 512 to 530 in 4c6b93d

	IoEngine::SpawnCoroutine(
	*strand,
	[strand, checkable, cmdLine, cr, psCommand, psHost, expectedSan, psPort, conn, req, start, checkTimeout](asio::yield_context yc) {
	Timeout::Ptr timeout = new Timeout(strand->context(), *strand, boost::posix_time::microseconds(int64_t(checkTimeout * 1e6)),
	[&conn, &checkable](boost::asio::yield_context yc) {
	Log(LogNotice, "IfwApiCheckTask")
	<< "Timeout while checking " << checkable->GetReflectionType()->GetName()
	<< " '" << checkable->GetName() << "', cancelling attempt";
	boost::system::error_code ec;
	conn->lowest_layer().cancel(ec);
	}
	);
	Defer cancelTimeout ([&timeout]() { timeout->Cancel(); });
	DoIfwNetIo(yc, checkable, cmdLine, cr, psCommand, psHost, expectedSan, psPort, *conn, *req, start);
	}
	);

Thus has no guarantee on which thread it's run, so I don't see how accessing that thread-local variable should do something useful.

[julianbrost]

PluginCheckTask also processes check results in a callable that may run somewhere else, but it actually took care of this:

icinga2/lib/methods/pluginchecktask.cpp

Lines 48 to 54 in 4c6b93d

	if (Checkable::ExecuteCommandProcessFinishedHandler) {
	callback = Checkable::ExecuteCommandProcessFinishedHandler;
	} else {
	callback = [checkable, cr](const Value& commandLine, const ProcessResult& pr) {
	ProcessFinishedHandler(checkable, cr, commandLine, pr);
	};
	}