hwmontempsensor: Simplify I/O and buffering
We can avoid the complexity and additional allocations of
boost::asio::streambuf by just using a simple fixed-size inline array as
the HwmonTempSensor read buffer.
While the underlying cause of the issue remains unknown at present, this
also seems to resolve a use-after-free bug that could occur with the
existing buffering scheme (https://github.com/openbmc/dbus-sensors/issues/20).
Based on a WIP patch by @edtanous:
https://gerrit.openbmc.org/c/openbmc/dbus-sensors/+/55041
Tested: on romed8hm3, hwmontempsensor provides appropriate readings on
dbus as it did prior to this patch. In combination with the debug
patches in https://github.com/openbmc/dbus-sensors/issues/20, an ASAN
build did not report any problems while repeatedly rebuilding sensors.
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
Signed-off-by: Ed Tanous <edtanous@google.com>
Change-Id: I7f0ab63fc398b8c938592f38f137174187c5e438
diff --git a/src/HwmonTempSensor.cpp b/src/HwmonTempSensor.cpp
index 7eb5e8f..744a40f 100644
--- a/src/HwmonTempSensor.cpp
+++ b/src/HwmonTempSensor.cpp
@@ -23,6 +23,7 @@
#include <sdbusplus/asio/connection.hpp>
#include <sdbusplus/asio/object_server.hpp>
+#include <charconv>
#include <iostream>
#include <istream>
#include <limits>
@@ -51,20 +52,12 @@
std::move(thresholdsIn), sensorConfiguration, objectType, false,
false, thisSensorParameters.maxValue, thisSensorParameters.minValue,
conn, powerState),
- objServer(objectServer), inputDev(io), waitTimer(io), path(path),
- offsetValue(thisSensorParameters.offsetValue),
+ objServer(objectServer),
+ inputDev(io, path, boost::asio::random_access_file::read_only),
+ waitTimer(io), path(path), offsetValue(thisSensorParameters.offsetValue),
scaleValue(thisSensorParameters.scaleValue),
sensorPollMs(static_cast<unsigned int>(pollRate * 1000))
{
- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
- int fd = open(path.c_str(), O_RDONLY);
- if (fd < 0)
- {
- std::cerr << "HwmonTempSensor " << sensorName << " failed to open "
- << path << "\n";
- }
- inputDev.assign(fd);
-
sensorInterface = objectServer.add_interface(
"/xyz/openbmc_project/sensors/" + thisSensorParameters.typeName + "/" +
name,
@@ -110,15 +103,15 @@
}
std::weak_ptr<HwmonTempSensor> weakRef = weak_from_this();
- boost::asio::async_read_until(inputDev, readBuf, '\n',
- [weakRef](const boost::system::error_code& ec,
- std::size_t /*bytes_transfered*/) {
+ inputDev.async_read_some_at(
+ 0, boost::asio::buffer(readBuf),
+ [weakRef](const boost::system::error_code& ec, std::size_t bytesRead) {
std::shared_ptr<HwmonTempSensor> self = weakRef.lock();
if (self)
{
- self->handleResponse(ec);
+ self->handleResponse(ec, bytesRead);
}
- });
+ });
}
void HwmonTempSensor::restartRead()
@@ -139,7 +132,8 @@
});
}
-void HwmonTempSensor::handleResponse(const boost::system::error_code& err)
+void HwmonTempSensor::handleResponse(const boost::system::error_code& err,
+ size_t bytesRead)
{
if ((err == boost::system::errc::bad_file_descriptor) ||
(err == boost::asio::error::misc_errors::not_found))
@@ -148,39 +142,27 @@
<< "\n";
return; // we're being destroyed
}
- std::istream responseStream(&readBuf);
+
if (!err)
{
- std::string response;
- std::getline(responseStream, response);
- try
- {
- rawValue = std::stod(response);
- double nvalue = (rawValue + offsetValue) * scaleValue;
- updateValue(nvalue);
- }
- catch (const std::invalid_argument&)
+ const char* bufEnd = readBuf.data() + bytesRead;
+ int nvalue = 0;
+ std::from_chars_result ret =
+ std::from_chars(readBuf.data(), bufEnd, nvalue);
+ if (ret.ec != std::errc())
{
incrementError();
}
+ else
+ {
+ updateValue((nvalue + offsetValue) * scaleValue);
+ }
}
else
{
incrementError();
}
- responseStream.clear();
- inputDev.close();
-
- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg)
- int fd = open(path.c_str(), O_RDONLY);
- if (fd < 0)
- {
- std::cerr << "Hwmon temp sensor " << name << " not valid " << path
- << "\n";
- return; // we're no longer valid
- }
- inputDev.assign(fd);
restartRead();
}