From 8fca30025603e1c918a0ecec31e58f235d4afbfb Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Tue, 30 Mar 2021 13:44:26 -0700 Subject: [PATCH] audio: Fix fixed size char array conversions For legacy HAL strings that are fixed size arrays the conversion code was using the array size for the HIDL string size. This lead to logging of error messages during reverse conversion. Fixed issue and refactored code to avoid duplication. Bug: 181269159 Test: atest android.hardware.audio.effect@7.0-util_tests also, verify that no error messages from EffectUtil appear during boot and audio playback Change-Id: Iac36ff33e65c502966ac2b7a4870cb5830545b23 --- .../all-versions/default/util/EffectUtils.cpp | 54 ++++++++++--------- .../default/util/tests/effectutils_tests.cpp | 17 ++++++ 2 files changed, 47 insertions(+), 24 deletions(-) diff --git a/audio/effect/all-versions/default/util/EffectUtils.cpp b/audio/effect/all-versions/default/util/EffectUtils.cpp index b4382dc2d6..1156d211a1 100644 --- a/audio/effect/all-versions/default/util/EffectUtils.cpp +++ b/audio/effect/all-versions/default/util/EffectUtils.cpp @@ -25,8 +25,6 @@ #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; @@ -154,6 +152,29 @@ status_t EffectUtils::effectConfigToHal(const EffectConfig& config, effect_confi return result; } +template +inline hidl_string charBufferFromHal(const char (&halBuf)[N]) { + // Even if the original field contains a non-terminated string, hidl_string + // adds a NUL terminator. + return hidl_string(halBuf, strnlen(halBuf, N)); +} + +template +inline status_t charBufferToHal(const hidl_string& str, char (&halBuf)[N], const char* fieldName) { + static_assert(N > 0); + const size_t halBufChars = N - 1; // Reserve one character for terminating NUL. + status_t result = NO_ERROR; + size_t strSize = str.size(); + if (strSize > halBufChars) { + ALOGE("%s is too long: %zu (%zu max)", fieldName, strSize, halBufChars); + strSize = halBufChars; + result = BAD_VALUE; + } + strncpy(halBuf, str.c_str(), strSize); + halBuf[strSize] = '\0'; + return result; +} + status_t EffectUtils::effectDescriptorFromHal(const effect_descriptor_t& halDescriptor, EffectDescriptor* descriptor) { UuidUtils::uuidFromHal(halDescriptor.type, &descriptor->type); @@ -166,9 +187,8 @@ status_t EffectUtils::effectDescriptorFromHal(const effect_descriptor_t& halDesc 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)); + descriptor->name = charBufferFromHal(halDescriptor.name); + descriptor->implementor = charBufferFromHal(halDescriptor.implementor); #endif return NO_ERROR; } @@ -186,25 +206,11 @@ status_t EffectUtils::effectDescriptorToHal(const EffectDescriptor& descriptor, memcpy(halDescriptor->implementor, descriptor.implementor.data(), descriptor.implementor.size()); #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'; + // According to 'dumpEffectDescriptor', 'name' and 'implementor' must be NUL-terminated. + CONVERT_CHECKED(charBufferToHal(descriptor.name, halDescriptor->name, "effect name"), result); + CONVERT_CHECKED(charBufferToHal(descriptor.implementor, halDescriptor->implementor, + "effect implementor"), + result); #endif return result; } 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 f3651de236..d021fa0b92 100644 --- a/audio/effect/all-versions/default/util/tests/effectutils_tests.cpp +++ b/audio/effect/all-versions/default/util/tests/effectutils_tests.cpp @@ -154,3 +154,20 @@ TEST(EffectUtils, ConvertDescriptor) { EXPECT_EQ(NO_ERROR, EffectUtils::effectDescriptorFromHal(halDesc, &descBack)); EXPECT_EQ(desc, descBack); } + +TEST(EffectUtils, ConvertNameAndImplementor) { + for (size_t i = 0; i < EFFECT_STRING_LEN_MAX; ++i) { + effect_descriptor_t halDesc{}; + for (size_t c = 0; c < i; ++c) { // '<' to accommodate NUL terminator. + halDesc.name[c] = halDesc.implementor[c] = 'A' + static_cast(c); + } + EffectDescriptor desc; + EXPECT_EQ(NO_ERROR, EffectUtils::effectDescriptorFromHal(halDesc, &desc)); + effect_descriptor_t halDescBack; + EXPECT_EQ(NO_ERROR, EffectUtils::effectDescriptorToHal(desc, &halDescBack)); + EXPECT_EQ(i, strlen(halDescBack.name)); + EXPECT_EQ(i, strlen(halDescBack.implementor)); + EXPECT_EQ(0, strcmp(halDesc.name, halDescBack.name)); + EXPECT_EQ(0, strcmp(halDesc.implementor, halDescBack.implementor)); + } +}