From 660a86de6aec5368e88c2834885433a88824fcce Mon Sep 17 00:00:00 2001
From: Mikhail Naganov <mnaganov@google.com>
Date: Fri, 12 Mar 2021 18:08:46 +0000
Subject: [PATCH] audio: Use strings for EffectDescriptor.{name|implementor}

The use of fixed size char arrays was a direct copy
of the C structure approach. In HIDL, the canonical approach
is to use strings.

Bug: 181883090
Test: atest android.hardware.audio.effect@7.0-util_tests
Change-Id: I468810e07e5ba8b3bd6f03c5acfa24009ef2e48a
---
 audio/effect/7.0/types.hal                    |  4 +--
 .../all-versions/default/util/EffectUtils.cpp | 36 ++++++++++++++++++-
 .../default/util/tests/effectutils_tests.cpp  | 12 +++++++
 3 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/audio/effect/7.0/types.hal b/audio/effect/7.0/types.hal
index bb2d7b3d06..8f4f885fd2 100644
--- a/audio/effect/7.0/types.hal
+++ b/audio/effect/7.0/types.hal
@@ -220,9 +220,9 @@ struct EffectDescriptor {
      */
     uint16_t memoryUsage;
     /** Human readable effect name. */
-    uint8_t[64] name;
+    string name;
     /** Human readable effect implementor name. */
-    uint8_t[64] implementor;
+    string implementor;
 };
 
 /**
diff --git a/audio/effect/all-versions/default/util/EffectUtils.cpp b/audio/effect/all-versions/default/util/EffectUtils.cpp
index 1c0419aba3..b4382dc2d6 100644
--- a/audio/effect/all-versions/default/util/EffectUtils.cpp
+++ b/audio/effect/all-versions/default/util/EffectUtils.cpp
@@ -16,12 +16,17 @@
 
 #include <memory.h>
 
+#define LOG_TAG "EffectUtils"
+#include <log/log.h>
+
 #include <HidlUtils.h>
 #include <UuidUtils.h>
 #include <common/all-versions/VersionUtils.h>
 
 #include "util/EffectUtils.h"
 
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof(*(a)))
+
 using ::android::hardware::audio::common::CPP_VERSION::implementation::HidlUtils;
 using ::android::hardware::audio::common::CPP_VERSION::implementation::UuidUtils;
 using ::android::hardware::audio::common::utils::EnumBitfield;
@@ -156,23 +161,52 @@ status_t EffectUtils::effectDescriptorFromHal(const effect_descriptor_t& halDesc
     descriptor->flags = EnumBitfield<EffectFlags>(halDescriptor.flags);
     descriptor->cpuLoad = halDescriptor.cpuLoad;
     descriptor->memoryUsage = halDescriptor.memoryUsage;
+#if MAJOR_VERSION <= 6
     memcpy(descriptor->name.data(), halDescriptor.name, descriptor->name.size());
     memcpy(descriptor->implementor.data(), halDescriptor.implementor,
            descriptor->implementor.size());
+#else
+    descriptor->name = hidl_string(halDescriptor.name, ARRAY_SIZE(halDescriptor.name));
+    descriptor->implementor =
+            hidl_string(halDescriptor.implementor, ARRAY_SIZE(halDescriptor.implementor));
+#endif
     return NO_ERROR;
 }
 
 status_t EffectUtils::effectDescriptorToHal(const EffectDescriptor& descriptor,
                                             effect_descriptor_t* halDescriptor) {
+    status_t result = NO_ERROR;
     UuidUtils::uuidToHal(descriptor.type, &halDescriptor->type);
     UuidUtils::uuidToHal(descriptor.uuid, &halDescriptor->uuid);
     halDescriptor->flags = static_cast<uint32_t>(descriptor.flags);
     halDescriptor->cpuLoad = descriptor.cpuLoad;
     halDescriptor->memoryUsage = descriptor.memoryUsage;
+#if MAJOR_VERSION <= 6
     memcpy(halDescriptor->name, descriptor.name.data(), descriptor.name.size());
     memcpy(halDescriptor->implementor, descriptor.implementor.data(),
            descriptor.implementor.size());
-    return NO_ERROR;
+#else
+    // According to 'dumpEffectDescriptor' 'name' and 'implementor' must be NUL-terminated.
+    size_t nameSize = descriptor.name.size();
+    if (nameSize >= ARRAY_SIZE(halDescriptor->name)) {
+        ALOGE("effect name is too long: %zu (%zu max)", nameSize,
+              ARRAY_SIZE(halDescriptor->name) - 1);
+        nameSize = ARRAY_SIZE(halDescriptor->name) - 1;
+        result = BAD_VALUE;
+    }
+    strncpy(halDescriptor->name, descriptor.name.c_str(), nameSize);
+    halDescriptor->name[nameSize] = '\0';
+    size_t implementorSize = descriptor.implementor.size();
+    if (implementorSize >= ARRAY_SIZE(halDescriptor->implementor)) {
+        ALOGE("effect implementor is too long: %zu (%zu max)", implementorSize,
+              ARRAY_SIZE(halDescriptor->implementor) - 1);
+        implementorSize = ARRAY_SIZE(halDescriptor->implementor) - 1;
+        result = BAD_VALUE;
+    }
+    strncpy(halDescriptor->implementor, descriptor.implementor.c_str(), implementorSize);
+    halDescriptor->implementor[implementorSize] = '\0';
+#endif
+    return result;
 }
 
 }  // namespace implementation
diff --git a/audio/effect/all-versions/default/util/tests/effectutils_tests.cpp b/audio/effect/all-versions/default/util/tests/effectutils_tests.cpp
index 7eb8cd2958..f3651de236 100644
--- a/audio/effect/all-versions/default/util/tests/effectutils_tests.cpp
+++ b/audio/effect/all-versions/default/util/tests/effectutils_tests.cpp
@@ -134,8 +134,20 @@ TEST(EffectUtils, ConvertBufferConfig) {
     EXPECT_EQ(format, formatBackIn);
 }
 
+TEST(EffectUtils, ConvertInvalidDescriptor) {
+    effect_descriptor_t halDesc;
+    EffectDescriptor longName{};
+    longName.name = std::string(EFFECT_STRING_LEN_MAX, 'x');
+    EXPECT_EQ(BAD_VALUE, EffectUtils::effectDescriptorToHal(longName, &halDesc));
+    EffectDescriptor longImplementor{};
+    longImplementor.implementor = std::string(EFFECT_STRING_LEN_MAX, 'x');
+    EXPECT_EQ(BAD_VALUE, EffectUtils::effectDescriptorToHal(longImplementor, &halDesc));
+}
+
 TEST(EffectUtils, ConvertDescriptor) {
     EffectDescriptor desc{};
+    desc.name = "test";
+    desc.implementor = "foo";
     effect_descriptor_t halDesc;
     EXPECT_EQ(NO_ERROR, EffectUtils::effectDescriptorToHal(desc, &halDesc));
     EffectDescriptor descBack;