Enhance validation and testing for reading from fw_ver

Summary:
- Replace the check for whitespace/control character with a check for any characters except alphanumeric and period/dash/underscore (.-_). (Leading/trailing whitespace is still permitted / ignored.)
- Check for too-long version strings. Length limit is 64 characters.
- Add some unit tests covering both new and existing cases.

The main reason to make these changes is to prevent issues with ODS keys, which have certain limitations:
https://www.internalfb.com/intern/wiki/ODS/ODS_User_Guide/Time_Series/Entity-Key_Name_Restrictions/

I expect fb303 or any other ODS library handles invalid keys gracefully, but even so in the best case there will be some data loss or bad data.

The new checks do not trip on any of the existing firmware versions (XXX.YYY.ZZZ, see also previous diff) and allow e.g. CRC checksums which we may support in the future.

Reviewed By: tao-ren

Differential Revision: D66527099

fbshipit-source-id: 5a3e4ad2f5da8c6ff3920150fb5f1493d7f49e24

fboss/platform/platform_manager/PlatformExplorer.cpp

@@ -66,6 +66,8 @@ std::string readVersionString(
       re2::RE2(PlatformExplorer::kFwVerXYPatternStr);
   static const re2::RE2 kFwVerXYZPattern =
       re2::RE2(PlatformExplorer::kFwVerXYZPatternStr);
+  static const re2::RE2 kFwVerValidCharsPattern =
+      re2::RE2(PlatformExplorer::kFwVerValidCharsPatternStr);
   const auto versionFileContent = platformFsUtils->getStringFileContent(path);
   if (!versionFileContent) {
     XLOGF(
@@ -75,15 +77,23 @@ std::string readVersionString(
         folly::errnoStr(errno));
     return fmt::format("ERROR_FILE_{}", errno);
   }
-  const auto versionString = versionFileContent.value();
+  const auto& versionString = versionFileContent.value();
   if (versionString.empty()) {
     XLOGF(ERR, "Empty firmware version file {}", path);
     return PlatformExplorer::kFwVerErrorEmptyFile;
   }
-  if (folly::hasSpaceOrCntrlSymbols(versionString)) {
+  if (versionString.length() > 64) {
     XLOGF(
         ERR,
-        "Firmware version string \"{}\" from file {} contains whitespace or control characters.",
+        "Firmware version \"{}\" from file {} is longer than 64 characters.",
+        versionString,
+        path);
+    return PlatformExplorer::kFwVerErrorInvalidString;
+  }
+  if (!re2::RE2::FullMatch(versionString, kFwVerValidCharsPattern)) {
+    XLOGF(
+        ERR,
+        "Firmware version \"{}\" from file {} contains invalid characters.",
         versionString,
         path);
     return PlatformExplorer::kFwVerErrorInvalidString;

fboss/platform/platform_manager/PlatformExplorer.h

@@ -24,6 +24,7 @@ class PlatformExplorer {
   auto static constexpr kFwVerXYPatternStr = R"((\d{1,3})\.(\d{1,3}))";
   auto static constexpr kFwVerXYZPatternStr =
       R"((\d{1,3})\.(\d{1,3})\.(\d{1,3}))";
+  auto static constexpr kFwVerValidCharsPatternStr = R"([a-zA-Z0-9\.\-_]+)";
 
   auto static constexpr kFirmwareVersion = "{}.firmware_version";
   auto static constexpr kGroupedFirmwareVersion = "{}.firmware_version.{}";

fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp

@@ -39,6 +39,16 @@ TEST(PlatformExplorerTest, PublishFirmwareVersions) {
   std::string cpldBadFwVerPath = "/run/devmap/cplds/TEST_CPLD_BADFWVER";
   EXPECT_TRUE(platformFsUtils->writeStringToFile(
       "123.456.789 // comment", fmt::format("{}/fw_ver", cpldBadFwVerPath)));
+  std::string cpldBadFwVer2Path = "/run/devmap/cplds/TEST_CPLD_BADFWVER2";
+  EXPECT_TRUE(platformFsUtils->writeStringToFile(
+      "123.456.$", fmt::format("{}/fw_ver", cpldBadFwVer2Path)));
+  std::string fpgaBadFwVerPath = "/run/devmap/fpgas/TEST_FPGA_BADFWVER";
+  EXPECT_TRUE(platformFsUtils->writeStringToFile(
+      "1.2.3 a", fmt::format("{}/fw_ver", fpgaBadFwVerPath)));
+  std::string fpgaLongFwVerPath = "/run/devmap/fpgas/TEST_FPGA_LONGFWVER";
+  std::string bigStr = "0123456789.0123456789.0123456789"; // 32 chars
+  EXPECT_TRUE(platformFsUtils->writeStringToFile(
+      bigStr + "." + bigStr, fmt::format("{}/fw_ver", fpgaLongFwVerPath)));
   // Case with fw_ver under hwmon
   std::string cpldHwmonFwVerPath = "/run/devmap/cplds/FAN0_CPLD_FWVER";
   EXPECT_TRUE(platformFsUtils->writeStringToFile(
@@ -58,6 +68,9 @@ TEST(PlatformExplorerTest, PublishFirmwareVersions) {
   platformConfig.symbolicLinkToDevicePath()[fpgaFwVerPath] = "";
   platformConfig.symbolicLinkToDevicePath()[cpldFwVerPath] = "";
   platformConfig.symbolicLinkToDevicePath()[cpldBadFwVerPath] = "";
+  platformConfig.symbolicLinkToDevicePath()[cpldBadFwVer2Path] = "";
+  platformConfig.symbolicLinkToDevicePath()[fpgaBadFwVerPath] = "";
+  platformConfig.symbolicLinkToDevicePath()[fpgaLongFwVerPath] = "";
   platformConfig.symbolicLinkToDevicePath()[cpldHwmonFwVerPath] = "";
   platformConfig.symbolicLinkToDevicePath()[cpldHwmonTrapPath] = "";
   platformConfig.symbolicLinkToDevicePath()[fpgaNonePath] = "";
@@ -67,7 +80,14 @@ TEST(PlatformExplorerTest, PublishFirmwareVersions) {
 
   expectVersions("TEST_FPGA_FWVER", "1.2");
   expectVersions("TEST_CPLD_FWVER", "123.456.789");
-  expectVersions("TEST_CPLD_BADFWVER", "ERROR_INVALID_STRING");
+  expectVersions(
+      "TEST_CPLD_BADFWVER", PlatformExplorer::kFwVerErrorInvalidString);
+  expectVersions(
+      "TEST_CPLD_BADFWVER2", PlatformExplorer::kFwVerErrorInvalidString);
+  expectVersions(
+      "TEST_FPGA_BADFWVER", PlatformExplorer::kFwVerErrorInvalidString);
+  expectVersions(
+      "TEST_FPGA_LONGFWVER", PlatformExplorer::kFwVerErrorInvalidString);
   expectVersions("FAN0_CPLD_FWVER", "1.2.3");
   expectVersions("TAHAN_SMB_CPLD_TRAP", "7.8.9");
   expectVersions("NONE", PlatformExplorer::kFwVerErrorFileNotFound);