Skip to content
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

IfwApiCheckTask uses thread-local variable in coroutine #10144

Closed
julianbrost opened this issue Aug 30, 2024 · 1 comment · Fixed by #10140
Closed

IfwApiCheckTask uses thread-local variable in coroutine #10144

julianbrost opened this issue Aug 30, 2024 · 1 comment · Fixed by #10140
Assignees
Labels
area/checks Check execution and results bug Something isn't working
Milestone

Comments

@julianbrost
Copy link
Contributor

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

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

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:

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

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

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:

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
Copy link
Contributor Author

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

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

@yhabteab yhabteab self-assigned this Aug 30, 2024
@yhabteab yhabteab added bug Something isn't working area/checks Check execution and results labels Aug 30, 2024
@yhabteab yhabteab added this to the 2.15.0 milestone Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/checks Check execution and results bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants