Enhance UEFI test code
Changes:
- variable_store_tests.cpp
- eliminate duplicated string size calculation
- simplify compare_variable_name(). The function is changed to fail
if size of the two string is not matching.
- There was a repeating pattern creating uint8_t msg_buffer on the
stack. This is test code, but a large enough name + data could
cause stack underrun. I added a vector and use it as a buffer.
This moves allocation to the HEAP and the compiler will take care
of cleaning up the memory when the variable goes out of scope.
- variable_index_tests.cpp
- eliminate duplicated string size calculation
- convert null_name to a class member to remove code duplication
Signed-off-by: Gyorgy Szing <Gyorgy.Szing@arm.com>
Change-Id: Ieb108bf84dde21b7659c90452c42c8e7428909e0
diff --git a/components/service/uefi/smm_variable/backend/test/variable_index_tests.cpp b/components/service/uefi/smm_variable/backend/test/variable_index_tests.cpp
index 507ad8a..1b7a6b8 100644
--- a/components/service/uefi/smm_variable/backend/test/variable_index_tests.cpp
+++ b/components/service/uefi/smm_variable/backend/test/variable_index_tests.cpp
@@ -44,6 +44,7 @@
name_1 = to_variable_name(u"var1");
name_2 = to_variable_name(u"var2_nv");
name_3 = to_variable_name(u"var3_nv");
+ null_name = to_variable_name(u"");
}
void teardown()
@@ -59,23 +60,28 @@
return var_name;
}
+ size_t string_get_size_in_bytes(const std::u16string &string)
+ {
+ return string.size() * sizeof(typename std::u16string::value_type);
+ }
+
void create_variables()
{
struct variable_info *info = NULL;
info = variable_index_add_entry(&m_variable_index, &guid_1,
- name_1.size() * sizeof(int16_t), (int16_t *) name_1.data());
+ string_get_size_in_bytes(name_1), (int16_t *) name_1.data());
CHECK_TRUE(info);
variable_index_set_variable(info, EFI_VARIABLE_BOOTSERVICE_ACCESS);
info = variable_index_add_entry(&m_variable_index, &guid_2,
- name_2.size() * sizeof(int16_t), (int16_t *) name_2.data());
+ string_get_size_in_bytes(name_2), (int16_t *) name_2.data());
CHECK_TRUE(info);
variable_index_set_variable(info, EFI_VARIABLE_NON_VOLATILE |
EFI_VARIABLE_BOOTSERVICE_ACCESS);
info = variable_index_add_entry(&m_variable_index, &guid_1,
- name_3.size() * sizeof(int16_t), (int16_t *) name_3.data());
+ string_get_size_in_bytes(name_3), (int16_t *) name_3.data());
CHECK_TRUE(info);
variable_index_set_variable(info, EFI_VARIABLE_NON_VOLATILE |
EFI_VARIABLE_RUNTIME_ACCESS |
@@ -90,6 +96,7 @@
std::u16string name_1;
std::u16string name_2;
std::u16string name_3;
+ std::u16string null_name;
};
TEST(UefiVariableIndexTests, emptyIndexOperations)
@@ -98,12 +105,12 @@
struct variable_info *info = NULL;
/* Expect not to find a variable */
- info = variable_index_find(&m_variable_index, &guid_1, name_1.size() * sizeof(int16_t),
+ info = variable_index_find(&m_variable_index, &guid_1, string_get_size_in_bytes(name_1),
(const int16_t *) name_1.data());
POINTERS_EQUAL(NULL, info);
/* Expect also find next to be rejected */
- info = variable_index_find_next(&m_variable_index, &guid_1, name_1.size() * sizeof(int16_t),
+ info = variable_index_find_next(&m_variable_index, &guid_1, string_get_size_in_bytes(name_1),
(const int16_t *) name_1.data(), &status);
POINTERS_EQUAL(NULL, info);
UNSIGNED_LONGLONGS_EQUAL(EFI_INVALID_PARAMETER, status);
@@ -120,7 +127,7 @@
name = to_variable_name(
u"a long variable name that exceeds the length limit with a few chars");
- info = variable_index_add_entry(&m_variable_index, &guid_1, name.size() * sizeof(int16_t),
+ info = variable_index_add_entry(&m_variable_index, &guid_1, string_get_size_in_bytes(name),
(int16_t *) name.data());
/* Expect the add to fail because of an oversized name */
@@ -128,7 +135,7 @@
name = to_variable_name(u"a long variable name that fits!");
- info = variable_index_add_entry(&m_variable_index, &guid_1, name.size() * sizeof(int16_t),
+ info = variable_index_add_entry(&m_variable_index, &guid_1, string_get_size_in_bytes(name),
(int16_t *) name.data());
/* Expect the add succeed */
@@ -143,7 +150,7 @@
/* Expect to be able to fill the index */
for (size_t i = 0; i < MAX_VARIABLES; ++i) {
info = variable_index_add_entry(&m_variable_index, &guid,
- name_1.size() * sizeof(int16_t), (int16_t *) name_1.data());
+ string_get_size_in_bytes(name_1), (int16_t *) name_1.data());
CHECK_TRUE(info);
@@ -152,7 +159,7 @@
}
/* Variable index should now be full */
- info = variable_index_add_entry(&m_variable_index, &guid, name_1.size() * sizeof(int16_t),
+ info = variable_index_add_entry(&m_variable_index, &guid, string_get_size_in_bytes(name_1),
(int16_t *) name_1.data());
POINTERS_EQUAL(NULL, info);
@@ -161,13 +168,12 @@
TEST(UefiVariableIndexTests, enumerateStore)
{
const struct variable_info *info = NULL;
- const std::u16string null_name = to_variable_name(u"");
efi_status_t status = EFI_NOT_FOUND;
create_variables();
info = variable_index_find_next(&m_variable_index, &guid_1,
- null_name.size() * sizeof(int16_t), (const int16_t *) null_name.data(),
+ string_get_size_in_bytes(null_name), (const int16_t *) null_name.data(),
&status);
CHECK_TRUE(info);
UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
@@ -232,10 +238,9 @@
/* Enumerate and now expect only NV variables to be present */
status = EFI_NOT_FOUND;
const struct variable_info *info = NULL;
- std::u16string null_name = to_variable_name(u"");
info = variable_index_find_next(&m_variable_index, &guid_1,
- null_name.size() * sizeof(int16_t), (const int16_t *) null_name.data(),
+ string_get_size_in_bytes(null_name), (const int16_t *) null_name.data(),
&status);
CHECK_TRUE(info);
UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
@@ -281,7 +286,7 @@
create_variables();
/* Remove one of the NV variables */
- info = variable_index_find(&m_variable_index, &guid_2, name_2.size() * sizeof(int16_t),
+ info = variable_index_find(&m_variable_index, &guid_2, string_get_size_in_bytes(name_2),
(const int16_t *) name_2.data());
variable_index_clear_variable(&m_variable_index, info);
@@ -294,7 +299,7 @@
UNSIGNED_LONGS_EQUAL((sizeof(struct variable_metadata) * 1), dump_len);
/* Remove the volatile variable */
- info = variable_index_find(&m_variable_index, &guid_1, name_1.size() * sizeof(int16_t),
+ info = variable_index_find(&m_variable_index, &guid_1, string_get_size_in_bytes(name_1),
(const int16_t *) name_1.data());
variable_index_clear_variable(&m_variable_index, info);
@@ -307,7 +312,7 @@
UNSIGNED_LONGS_EQUAL((sizeof(struct variable_metadata) * 1), dump_len);
/* Remove the remaining NV variable */
- info = variable_index_find(&m_variable_index, &guid_1, name_3.size() * sizeof(int16_t),
+ info = variable_index_find(&m_variable_index, &guid_1, string_get_size_in_bytes(name_3),
(const int16_t *) name_3.data());
variable_index_clear_variable(&m_variable_index, info);
@@ -322,10 +327,9 @@
/* Enumerate and now expect an empty index */
info = NULL;
efi_status_t status = EFI_SUCCESS;
- std::u16string null_name = to_variable_name(u"");
info = variable_index_find_next(&m_variable_index, &guid_1,
- null_name.size() * sizeof(int16_t), (const int16_t *) null_name.data(),
+ string_get_size_in_bytes(null_name), (const int16_t *) null_name.data(),
&status);
POINTERS_EQUAL(NULL, info);
UNSIGNED_LONGLONGS_EQUAL(EFI_NOT_FOUND, status);
@@ -345,7 +349,7 @@
/* Check first entry is as expected */
info = variable_index_iterator_current(&iter);
CHECK_TRUE(info);
- UNSIGNED_LONGS_EQUAL(name_1.size() * sizeof(int16_t), info->metadata.name_size);
+ UNSIGNED_LONGS_EQUAL(string_get_size_in_bytes(name_1), info->metadata.name_size);
MEMCMP_EQUAL(name_1.data(), info->metadata.name, info->metadata.name_size);
variable_index_iterator_next(&iter);
@@ -354,7 +358,7 @@
/* Check next is as expected */
info = variable_index_iterator_current(&iter);
CHECK_TRUE(info);
- UNSIGNED_LONGS_EQUAL(name_2.size() * sizeof(int16_t), info->metadata.name_size);
+ UNSIGNED_LONGS_EQUAL(string_get_size_in_bytes(name_2), info->metadata.name_size);
MEMCMP_EQUAL(name_2.data(), info->metadata.name, info->metadata.name_size);
struct variable_info *info_to_remove = info;
@@ -365,7 +369,7 @@
/* Check next is as expected */
info = variable_index_iterator_current(&iter);
CHECK_TRUE(info);
- UNSIGNED_LONGS_EQUAL(name_3.size() * sizeof(int16_t), info->metadata.name_size);
+ UNSIGNED_LONGS_EQUAL(string_get_size_in_bytes(name_3), info->metadata.name_size);
MEMCMP_EQUAL(name_3.data(), info->metadata.name, info->metadata.name_size);
/* Expect iterating to be done */
@@ -383,7 +387,7 @@
/* Check first entry is as expected */
info = variable_index_iterator_current(&iter);
CHECK_TRUE(info);
- UNSIGNED_LONGS_EQUAL(name_1.size() * sizeof(int16_t), info->metadata.name_size);
+ UNSIGNED_LONGS_EQUAL(string_get_size_in_bytes(name_1), info->metadata.name_size);
MEMCMP_EQUAL(name_1.data(), info->metadata.name, info->metadata.name_size);
variable_index_iterator_next(&iter);
@@ -392,7 +396,7 @@
/* Check next entry is as expected */
info = variable_index_iterator_current(&iter);
CHECK_TRUE(info);
- UNSIGNED_LONGS_EQUAL(name_3.size() * sizeof(int16_t), info->metadata.name_size);
+ UNSIGNED_LONGS_EQUAL(string_get_size_in_bytes(name_3), info->metadata.name_size);
MEMCMP_EQUAL(name_3.data(), info->metadata.name, info->metadata.name_size);
/* Expect iterating to be done */
@@ -418,7 +422,7 @@
/* Set check constraints on one of the variables */
struct variable_info *info = variable_index_find(
- &m_variable_index, &guid_2, name_2.size() * sizeof(int16_t), (const int16_t *) name_2.data());
+ &m_variable_index, &guid_2, string_get_size_in_bytes(name_2), (const int16_t *) name_2.data());
CHECK_TRUE(info);
CHECK_TRUE(info->is_variable_set);
@@ -434,7 +438,7 @@
*/
variable_index_clear_variable(&m_variable_index, info);
- info = variable_index_find(&m_variable_index, &guid_2, name_2.size() * sizeof(int16_t),
+ info = variable_index_find(&m_variable_index, &guid_2, string_get_size_in_bytes(name_2),
(const int16_t *) name_2.data());
CHECK_TRUE(info);
@@ -444,10 +448,9 @@
/* Enumerate over variables, only expecting to find the two remaining 'set' variables. */
info = NULL;
efi_status_t status = EFI_NOT_FOUND;
- std::u16string null_name = to_variable_name(u"");
info = variable_index_find_next(&m_variable_index, &guid_1,
- null_name.size() * sizeof(int16_t), (const int16_t *) null_name.data(),
+ string_get_size_in_bytes(null_name), (const int16_t *) null_name.data(),
&status);
CHECK_TRUE(info);
UNSIGNED_LONGLONGS_EQUAL(EFI_SUCCESS, status);
@@ -477,7 +480,7 @@
/* Check first entry is as expected */
info = variable_index_iterator_current(&iter);
CHECK_TRUE(info);
- UNSIGNED_LONGS_EQUAL(name_1.size() * sizeof(int16_t), info->metadata.name_size);
+ UNSIGNED_LONGS_EQUAL(string_get_size_in_bytes(name_1), info->metadata.name_size);
MEMCMP_EQUAL(name_1.data(), info->metadata.name, info->metadata.name_size);
variable_index_iterator_next(&iter);
@@ -486,7 +489,7 @@
/* Check next is as expected */
info = variable_index_iterator_current(&iter);
CHECK_TRUE(info);
- UNSIGNED_LONGS_EQUAL(name_2.size() * sizeof(int16_t), info->metadata.name_size);
+ UNSIGNED_LONGS_EQUAL(string_get_size_in_bytes(name_2), info->metadata.name_size);
MEMCMP_EQUAL(name_2.data(), info->metadata.name, info->metadata.name_size);
variable_index_iterator_next(&iter);
@@ -495,7 +498,7 @@
/* Check next is as expected */
info = variable_index_iterator_current(&iter);
CHECK_TRUE(info);
- UNSIGNED_LONGS_EQUAL(name_3.size() * sizeof(int16_t), info->metadata.name_size);
+ UNSIGNED_LONGS_EQUAL(string_get_size_in_bytes(name_3), info->metadata.name_size);
MEMCMP_EQUAL(name_3.data(), info->metadata.name, info->metadata.name_size);
/* Expect iterating to be done */
@@ -517,14 +520,15 @@
/* Initially expect no variable_info */
struct variable_info *info = variable_index_find(
- &m_variable_index, &guid_2, name_2.size() * sizeof(int16_t),
+ &m_variable_index, &guid_2, string_get_size_in_bytes(name_2),
(const int16_t *) name_2.data());
CHECK_FALSE(info);
/* Adding the check constraints should result in an entry being added */
- info = variable_index_add_entry(&m_variable_index, &guid_2, name_2.size() * sizeof(int16_t),
- (const int16_t *) name_2.data());
+ info = variable_index_add_entry(
+ &m_variable_index, &guid_2, string_get_size_in_bytes(name_2),
+ (const int16_t *) name_2.data());
CHECK_TRUE(info);
variable_index_set_constraints(info, &constraints);
diff --git a/components/service/uefi/smm_variable/backend/test/variable_store_tests.cpp b/components/service/uefi/smm_variable/backend/test/variable_store_tests.cpp
index 6235c00..0d3450e 100644
--- a/components/service/uefi/smm_variable/backend/test/variable_store_tests.cpp
+++ b/components/service/uefi/smm_variable/backend/test/variable_store_tests.cpp
@@ -56,32 +56,34 @@
return var_name;
}
+ size_t string_get_size_in_bytes(const std::u16string &string)
+ {
+ return string.size() * sizeof(uint16_t);
+ }
+
bool compare_variable_name(const std::u16string &expected, const int16_t *name,
size_t name_size)
{
- bool is_equal = (expected.size() + 1 <= name_size / sizeof(int16_t));
+ std::u16string var_name = to_variable_name(expected);
- for (size_t i = 0; is_equal && i < expected.size(); i++) {
- if (name[i] != (int16_t)expected[i]) {
- is_equal = false;
- break;
- }
- }
+ if (name_size != string_get_size_in_bytes(var_name))
+ return false;
- return is_equal;
+ return 0==memcmp(name, var_name.data(), name_size);
}
efi_status_t set_variable(const std::u16string &name, const uint8_t *data, size_t data_size,
uint32_t attributes, EFI_GUID *guid = NULL)
{
std::u16string var_name = to_variable_name(name);
- size_t name_size = var_name.size() * sizeof(int16_t);
+ size_t name_size = string_get_size_in_bytes(var_name);
- uint8_t msg_buffer[SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_SIZE(name_size,
- data_size)];
+ /* Use a vector as a temporary buffer to move allocation to the HEAP and for RAII benefits. */
+ std::vector<uint8_t> msg_buffer(
+ (std::size_t)SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_SIZE(name_size, data_size));
SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *access_variable =
- (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *)msg_buffer;
+ (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) msg_buffer.data();
if (!guid)
guid = &m_common_guid;
@@ -92,9 +94,9 @@
memcpy(access_variable->Name, var_name.data(), name_size);
access_variable->DataSize = data_size;
- memcpy(&msg_buffer[SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(
- access_variable)],
- data, data_size);
+ memcpy(msg_buffer.data() +
+ SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(access_variable),
+ data, data_size);
efi_status_t status =
uefi_variable_store_set_variable(&m_uefi_variable_store, access_variable);
@@ -105,7 +107,7 @@
efi_status_t set_variable(const std::u16string &name, const std::string &data,
uint32_t attributes, EFI_GUID *guid = NULL)
{
- return set_variable(name, static_cast<uint8_t *>(data.data()), data.size(),
+ return set_variable(name, (uint8_t *) data.data(), data.size(),
attributes, guid);
}
@@ -113,12 +115,13 @@
size_t data_len_clamp = VARIABLE_BUFFER_SIZE)
{
std::u16string var_name = to_variable_name(name);
- size_t name_size = var_name.size() * sizeof(uint16_t);
+ size_t name_size = string_get_size_in_bytes(var_name);
+
size_t total_size = 0;
- uint8_t msg_buffer[VARIABLE_BUFFER_SIZE];
+ std::vector<uint8_t> msg_buffer(VARIABLE_BUFFER_SIZE);
SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *access_variable =
- (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *)msg_buffer;
+ (SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE *) msg_buffer.data();
access_variable->Guid = m_common_guid;
access_variable->Attributes = 0;
@@ -142,7 +145,7 @@
if (status == EFI_SUCCESS) {
const char *data_start =
- (const char *)(msg_buffer +
+ (const char *) (msg_buffer.data() +
SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_DATA_OFFSET(
access_variable));
@@ -192,11 +195,13 @@
const VAR_CHECK_VARIABLE_PROPERTY &check_property)
{
std::u16string var_name = to_variable_name(name);
- size_t name_size = var_name.size() * sizeof(uint16_t);
+ size_t name_size = string_get_size_in_bytes(var_name);
+ std::vector<uint8_t> msg_buffer(
+ SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY_SIZE(name_size));
SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY *check_var =
- (SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY *)msg_buffer;
+ (SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY *) msg_buffer.data();
check_var->Guid = m_common_guid;
check_var->NameSize = name_size;
@@ -213,7 +218,7 @@
void zap_stored_variable(const std::u16string &name)
{
std::u16string var_name = to_variable_name(name);
- size_t name_size = var_name.size() * sizeof(int16_t);
+ size_t name_size = string_get_size_in_bytes(var_name);
/* Create the condition where a variable is indexed but
* there is no corresponding stored object.
@@ -493,9 +498,9 @@
UNSIGNED_LONGLONGS_EQUAL(EFI_NOT_FOUND, status);
/* Expect access to be blocked for get_next_variable_name */
- uint8_t msg_buffer[VARIABLE_BUFFER_SIZE];
+ std::vector<uint8_t> msg_buffer(VARIABLE_BUFFER_SIZE);
SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *next_name =
- (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *)msg_buffer;
+ (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) msg_buffer.data();
size_t max_name_len =
VARIABLE_BUFFER_SIZE - SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_NAME_OFFSET;
@@ -566,15 +571,15 @@
/* Prepare to enumerate */
size_t total_len = 0;
- uint8_t msg_buffer[VARIABLE_BUFFER_SIZE];
+ std::vector<uint8_t> msg_buffer(VARIABLE_BUFFER_SIZE);
SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *next_name =
- (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *)msg_buffer;
+ (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) msg_buffer.data();
size_t max_name_len =
VARIABLE_BUFFER_SIZE - SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_NAME_OFFSET;
/* First check handling of invalid variable name */
std::u16string bogus_name = to_variable_name(u"bogus_variable");
- size_t bogus_name_size = bogus_name.size() * sizeof(uint16_t);
+ size_t bogus_name_size = string_get_size_in_bytes(bogus_name);
next_name->Guid = m_common_guid;
next_name->NameSize = bogus_name_size;
memcpy(next_name->Name, bogus_name.data(), bogus_name_size);
@@ -658,9 +663,9 @@
* to have gone and for the index to have been cleaned up
* for the failed variable 3.
*/
- uint8_t msg_buffer[VARIABLE_BUFFER_SIZE];
+ std::vector<uint8_t> msg_buffer(VARIABLE_BUFFER_SIZE);
SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *next_name =
- (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *)msg_buffer;
+ (SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME *) msg_buffer.data();
size_t max_name_len =
VARIABLE_BUFFER_SIZE - SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE_NAME_OFFSET;