[PATCH] efi_loader: fix string table reallocations

[PATCH] efi_loader: fix string table reallocations

[Vincent Stehlé]

The new_string() and set_string() HII string protocol functions keep
using old pointers after calling realloc(), which may have moved.

The set_string() function also assumes that reallocation only grew the
array by one element when computing the start of the memory to clear,
and forgets to update nstrings to the new table size after reallocation.

Repair and factorize the reallocation code in a new grow_string_table()
function to fix those issues.

Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Tom Rini <trini@konsulko.com>
---
This can be tested on the sandbox with the following command:

  ./u-boot -T -c 'setenv efi_selftest HII database protocols; bootefi selftest'

While the unit test does cause reallocation with new_string(), it does not
with set_string().

Further testing can be done by forcing stbl->strings = NULL, just after
calling realloc(stbl->strings, ...).
---
 lib/efi_loader/efi_hii.c | 64 +++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
index 7bf51ad43d1..90dff91e5d4 100644
--- a/lib/efi_loader/efi_hii.c
+++ b/lib/efi_loader/efi_hii.c
@@ -822,6 +822,37 @@ static bool language_match(char *language, char *languages)
 	return false;
 }
 
+/**
+ * grow_string_table() - Grow a string table if necessary.
+ * @stbl: String table to grow.
+ * @n:    Minimum table size desired.
+ *
+ * Make sure a string table can contain at least n elements.
+ * If the table is too small, reallocate the memory and clear the new elements.
+ *
+ * Return: false in case of reallocation error, true otherwise.
+ * In case of error, the original string table is left untouched.
+ * In case of success, stbl->strings and stbl->nstrings may have been updated.
+ */
+static bool grow_string_table(struct efi_string_table *stbl, u32 n)
+{
+	void *buf;
+	u32 old_n = stbl->nstrings;
+
+	if (old_n >= n)
+		return true;
+
+	buf = realloc(stbl->strings, sizeof(stbl->strings[0]) * n);
+	if (!buf)
+		return false;
+
+	stbl->strings = buf;
+	stbl->nstrings = n;
+	memset(&stbl->strings[old_n], 0, sizeof(stbl->strings[0]) * (n - old_n));
+
+	return true;
+}
+
 static efi_status_t EFIAPI
 new_string(const struct efi_hii_string_protocol *this,
 	   efi_hii_handle_t package_list,
@@ -847,23 +878,12 @@ new_string(const struct efi_hii_string_protocol *this,
 	list_for_each_entry(stbl, &hii->string_tables, link) {
 		if (language_match((char *)language, stbl->language)) {
 			efi_string_id_t new_id;
-			void *buf;
 			efi_string_t str;
 
 			new_id = ++hii->max_string_id;
-			if (stbl->nstrings < new_id) {
-				buf = realloc(stbl->strings,
-					      sizeof(stbl->strings[0])
-						* new_id);
-				if (!buf)
-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
-
-				memset(&stbl->strings[stbl->nstrings], 0,
-				       (new_id - stbl->nstrings)
-					 * sizeof(stbl->strings[0]));
-				stbl->strings = buf;
-				stbl->nstrings = new_id;
-			}
+
+			if (!grow_string_table(stbl, new_id))
+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
 
 			str = u16_strdup(string);
 			if (!str)
@@ -957,20 +977,8 @@ set_string(const struct efi_hii_string_protocol *this,
 			if (hii->max_string_id < string_id)
 				return EFI_EXIT(EFI_NOT_FOUND);
 
-			if (stbl->nstrings < string_id) {
-				void *buf;
-
-				buf = realloc(stbl->strings,
-					      string_id
-						* sizeof(stbl->strings[0]));
-				if (!buf)
-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
-
-				memset(&stbl->strings[string_id - 1], 0,
-				       (string_id - stbl->nstrings)
-					 * sizeof(stbl->strings[0]));
-				stbl->strings = buf;
-			}
+			if (!grow_string_table(stbl, string_id))
+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
 
 			str = u16_strdup(string);
 			if (!str)

---
base-commit: 2c7eaee7b3422ce7c84e892abfe16e1afce42e83
change-id: 20260916-ptr-670be74dcbd3

Best regards,
--  
Vincent.

Re: [PATCH] efi_loader: fix string table reallocations

[Heinrich Schuchardt]

Am 17. September 2026 11:45:31 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
>The new_string() and set_string() HII string protocol functions keep
>using old pointers after calling realloc(), which may have moved.
>
>The set_string() function also assumes that reallocation only grew the
>array by one element when computing the start of the memory to clear,
>and forgets to update nstrings to the new table size after reallocation.
>
>Repair and factorize the reallocation code in a new grow_string_table()
>function to fix those issues.


Should we add a test in U-Boot and possibly in SCT?

Best regards

Heinrich

>
>Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
>Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
>Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>Cc: Tom Rini <trini@konsulko.com>
>---
>This can be tested on the sandbox with the following command:
>
>  ./u-boot -T -c 'setenv efi_selftest HII database protocols; bootefi selftest'
>
>While the unit test does cause reallocation with new_string(), it does not
>with set_string().
>
>Further testing can be done by forcing stbl->strings = NULL, just after
>calling realloc(stbl->strings, ...).
>---
> lib/efi_loader/efi_hii.c | 64 +++++++++++++++++++++++++++---------------------
> 1 file changed, 36 insertions(+), 28 deletions(-)
>
>diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
>index 7bf51ad43d1..90dff91e5d4 100644
>--- a/lib/efi_loader/efi_hii.c
>+++ b/lib/efi_loader/efi_hii.c
>@@ -822,6 +822,37 @@ static bool language_match(char *language, char *languages)
> 	return false;
> }
> 
>+/**
>+ * grow_string_table() - Grow a string table if necessary.
>+ * @stbl: String table to grow.
>+ * @n:    Minimum table size desired.
>+ *
>+ * Make sure a string table can contain at least n elements.
>+ * If the table is too small, reallocate the memory and clear the new elements.
>+ *
>+ * Return: false in case of reallocation error, true otherwise.
>+ * In case of error, the original string table is left untouched.
>+ * In case of success, stbl->strings and stbl->nstrings may have been updated.
>+ */
>+static bool grow_string_table(struct efi_string_table *stbl, u32 n)
>+{
>+	void *buf;
>+	u32 old_n = stbl->nstrings;
>+
>+	if (old_n >= n)
>+		return true;
>+
>+	buf = realloc(stbl->strings, sizeof(stbl->strings[0]) * n);
>+	if (!buf)
>+		return false;
>+
>+	stbl->strings = buf;
>+	stbl->nstrings = n;
>+	memset(&stbl->strings[old_n], 0, sizeof(stbl->strings[0]) * (n - old_n));
>+
>+	return true;
>+}
>+
> static efi_status_t EFIAPI
> new_string(const struct efi_hii_string_protocol *this,
> 	   efi_hii_handle_t package_list,
>@@ -847,23 +878,12 @@ new_string(const struct efi_hii_string_protocol *this,
> 	list_for_each_entry(stbl, &hii->string_tables, link) {
> 		if (language_match((char *)language, stbl->language)) {
> 			efi_string_id_t new_id;
>-			void *buf;
> 			efi_string_t str;
> 
> 			new_id = ++hii->max_string_id;
>-			if (stbl->nstrings < new_id) {
>-				buf = realloc(stbl->strings,
>-					      sizeof(stbl->strings[0])
>-						* new_id);
>-				if (!buf)
>-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>-
>-				memset(&stbl->strings[stbl->nstrings], 0,
>-				       (new_id - stbl->nstrings)
>-					 * sizeof(stbl->strings[0]));
>-				stbl->strings = buf;
>-				stbl->nstrings = new_id;
>-			}
>+
>+			if (!grow_string_table(stbl, new_id))
>+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> 
> 			str = u16_strdup(string);
> 			if (!str)
>@@ -957,20 +977,8 @@ set_string(const struct efi_hii_string_protocol *this,
> 			if (hii->max_string_id < string_id)
> 				return EFI_EXIT(EFI_NOT_FOUND);
> 
>-			if (stbl->nstrings < string_id) {
>-				void *buf;
>-
>-				buf = realloc(stbl->strings,
>-					      string_id
>-						* sizeof(stbl->strings[0]));
>-				if (!buf)
>-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>-
>-				memset(&stbl->strings[string_id - 1], 0,
>-				       (string_id - stbl->nstrings)
>-					 * sizeof(stbl->strings[0]));
>-				stbl->strings = buf;
>-			}
>+			if (!grow_string_table(stbl, string_id))
>+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> 
> 			str = u16_strdup(string);
> 			if (!str)
>
>---
>base-commit: 2c7eaee7b3422ce7c84e892abfe16e1afce42e83
>change-id: 20260916-ptr-670be74dcbd3
>
>Best regards,
>--  
>Vincent.
>

Re: [PATCH] efi_loader: fix string table reallocations

[Vincent Stehlé]

On Thu, Sep 17, 2026 at 06:23:11PM +0200, Heinrich Schuchardt wrote:
> Am 17. September 2026 11:45:31 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
> >The new_string() and set_string() HII string protocol functions keep
> >using old pointers after calling realloc(), which may have moved.
> >
> >The set_string() function also assumes that reallocation only grew the
> >array by one element when computing the start of the memory to clear,
> >and forgets to update nstrings to the new table size after reallocation.
> >
> >Repair and factorize the reallocation code in a new grow_string_table()
> >function to fix those issues.
> 
> 
> Should we add a test in U-Boot and possibly in SCT?

Hi Heinrich,

Thanks for having a look.

I think the existing "HII database protocols" self-test is sufficient, thanks to
the change.

With this test, before the patch, the code coverage is fairly good in
new_string() but is poor in set_string().

After the patch, the code coverage of grow_string_table() is fairly good as
expected, and the poor code coverage of set_string() is "gone".

Best regards,
Vincent.

> 
> Best regards
> 
> Heinrich
> 
> >
> >Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
> >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> >Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >Cc: Tom Rini <trini@konsulko.com>
> >---
> >This can be tested on the sandbox with the following command:
> >
> >  ./u-boot -T -c 'setenv efi_selftest HII database protocols; bootefi selftest'
> >
> >While the unit test does cause reallocation with new_string(), it does not
> >with set_string().
> >
> >Further testing can be done by forcing stbl->strings = NULL, just after
> >calling realloc(stbl->strings, ...).
> >---
> > lib/efi_loader/efi_hii.c | 64 +++++++++++++++++++++++++++---------------------
> > 1 file changed, 36 insertions(+), 28 deletions(-)
> >
> >diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
> >index 7bf51ad43d1..90dff91e5d4 100644
> >--- a/lib/efi_loader/efi_hii.c
> >+++ b/lib/efi_loader/efi_hii.c
> >@@ -822,6 +822,37 @@ static bool language_match(char *language, char *languages)
> > 	return false;
> > }
> > 
> >+/**
> >+ * grow_string_table() - Grow a string table if necessary.
> >+ * @stbl: String table to grow.
> >+ * @n:    Minimum table size desired.
> >+ *
> >+ * Make sure a string table can contain at least n elements.
> >+ * If the table is too small, reallocate the memory and clear the new elements.
> >+ *
> >+ * Return: false in case of reallocation error, true otherwise.
> >+ * In case of error, the original string table is left untouched.
> >+ * In case of success, stbl->strings and stbl->nstrings may have been updated.
> >+ */
> >+static bool grow_string_table(struct efi_string_table *stbl, u32 n)
> >+{
> >+	void *buf;
> >+	u32 old_n = stbl->nstrings;
> >+
> >+	if (old_n >= n)
> >+		return true;
> >+
> >+	buf = realloc(stbl->strings, sizeof(stbl->strings[0]) * n);
> >+	if (!buf)
> >+		return false;
> >+
> >+	stbl->strings = buf;
> >+	stbl->nstrings = n;
> >+	memset(&stbl->strings[old_n], 0, sizeof(stbl->strings[0]) * (n - old_n));
> >+
> >+	return true;
> >+}
> >+
> > static efi_status_t EFIAPI
> > new_string(const struct efi_hii_string_protocol *this,
> > 	   efi_hii_handle_t package_list,
> >@@ -847,23 +878,12 @@ new_string(const struct efi_hii_string_protocol *this,
> > 	list_for_each_entry(stbl, &hii->string_tables, link) {
> > 		if (language_match((char *)language, stbl->language)) {
> > 			efi_string_id_t new_id;
> >-			void *buf;
> > 			efi_string_t str;
> > 
> > 			new_id = ++hii->max_string_id;
> >-			if (stbl->nstrings < new_id) {
> >-				buf = realloc(stbl->strings,
> >-					      sizeof(stbl->strings[0])
> >-						* new_id);
> >-				if (!buf)
> >-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> >-
> >-				memset(&stbl->strings[stbl->nstrings], 0,
> >-				       (new_id - stbl->nstrings)
> >-					 * sizeof(stbl->strings[0]));
> >-				stbl->strings = buf;
> >-				stbl->nstrings = new_id;
> >-			}
> >+
> >+			if (!grow_string_table(stbl, new_id))
> >+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> > 
> > 			str = u16_strdup(string);
> > 			if (!str)
> >@@ -957,20 +977,8 @@ set_string(const struct efi_hii_string_protocol *this,
> > 			if (hii->max_string_id < string_id)
> > 				return EFI_EXIT(EFI_NOT_FOUND);
> > 
> >-			if (stbl->nstrings < string_id) {
> >-				void *buf;
> >-
> >-				buf = realloc(stbl->strings,
> >-					      string_id
> >-						* sizeof(stbl->strings[0]));
> >-				if (!buf)
> >-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> >-
> >-				memset(&stbl->strings[string_id - 1], 0,
> >-				       (string_id - stbl->nstrings)
> >-					 * sizeof(stbl->strings[0]));
> >-				stbl->strings = buf;
> >-			}
> >+			if (!grow_string_table(stbl, string_id))
> >+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> > 
> > 			str = u16_strdup(string);
> > 			if (!str)
> >
> >---
> >base-commit: 2c7eaee7b3422ce7c84e892abfe16e1afce42e83
> >change-id: 20260916-ptr-670be74dcbd3
> >
> >Best regards,
> >--  
> >Vincent.
> >
> 

-- 
Vincent.

Re: [PATCH] efi_loader: fix string table reallocations

[Heinrich Schuchardt]

Am 18. September 2026 10:51:42 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
>On Thu, Sep 17, 2026 at 06:23:11PM +0200, Heinrich Schuchardt wrote:
>> Am 17. September 2026 11:45:31 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
>> >The new_string() and set_string() HII string protocol functions keep
>> >using old pointers after calling realloc(), which may have moved.
>> >
>> >The set_string() function also assumes that reallocation only grew the
>> >array by one element when computing the start of the memory to clear,
>> >and forgets to update nstrings to the new table size after reallocation.
>> >
>> >Repair and factorize the reallocation code in a new grow_string_table()
>> >function to fix those issues.
>> 
>> 
>> Should we add a test in U-Boot and possibly in SCT?
>
>Hi Heinrich,
>
>Thanks for having a look.
>
>I think the existing "HII database protocols" self-test is sufficient, thanks to
>the change.
>
>With this test, before the patch, the code coverage is fairly good in
>new_string() but is poor in set_string().
>
>After the patch, the code coverage of grow_string_table() is fairly good as
>expected, and the poor code coverage of set_string() is "gone".
>
>Best regards,
>Vincent.

If the test did not fail before your code was added, there must be some test case missing.

Best regards

Heinrich


>
>> 
>> Best regards
>> 
>> Heinrich
>> 
>> >
>> >Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
>> >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
>> >Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> >Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> >Cc: Tom Rini <trini@konsulko.com>
>> >---
>> >This can be tested on the sandbox with the following command:
>> >
>> >  ./u-boot -T -c 'setenv efi_selftest HII database protocols; bootefi selftest'
>> >
>> >While the unit test does cause reallocation with new_string(), it does not
>> >with set_string().
>> >
>> >Further testing can be done by forcing stbl->strings = NULL, just after
>> >calling realloc(stbl->strings, ...).
>> >---
>> > lib/efi_loader/efi_hii.c | 64 +++++++++++++++++++++++++++---------------------
>> > 1 file changed, 36 insertions(+), 28 deletions(-)
>> >
>> >diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
>> >index 7bf51ad43d1..90dff91e5d4 100644
>> >--- a/lib/efi_loader/efi_hii.c
>> >+++ b/lib/efi_loader/efi_hii.c
>> >@@ -822,6 +822,37 @@ static bool language_match(char *language, char *languages)
>> > 	return false;
>> > }
>> > 
>> >+/**
>> >+ * grow_string_table() - Grow a string table if necessary.
>> >+ * @stbl: String table to grow.
>> >+ * @n:    Minimum table size desired.
>> >+ *
>> >+ * Make sure a string table can contain at least n elements.
>> >+ * If the table is too small, reallocate the memory and clear the new elements.
>> >+ *
>> >+ * Return: false in case of reallocation error, true otherwise.
>> >+ * In case of error, the original string table is left untouched.
>> >+ * In case of success, stbl->strings and stbl->nstrings may have been updated.
>> >+ */
>> >+static bool grow_string_table(struct efi_string_table *stbl, u32 n)
>> >+{
>> >+	void *buf;
>> >+	u32 old_n = stbl->nstrings;
>> >+
>> >+	if (old_n >= n)
>> >+		return true;
>> >+
>> >+	buf = realloc(stbl->strings, sizeof(stbl->strings[0]) * n);
>> >+	if (!buf)
>> >+		return false;
>> >+
>> >+	stbl->strings = buf;
>> >+	stbl->nstrings = n;
>> >+	memset(&stbl->strings[old_n], 0, sizeof(stbl->strings[0]) * (n - old_n));
>> >+
>> >+	return true;
>> >+}
>> >+
>> > static efi_status_t EFIAPI
>> > new_string(const struct efi_hii_string_protocol *this,
>> > 	   efi_hii_handle_t package_list,
>> >@@ -847,23 +878,12 @@ new_string(const struct efi_hii_string_protocol *this,
>> > 	list_for_each_entry(stbl, &hii->string_tables, link) {
>> > 		if (language_match((char *)language, stbl->language)) {
>> > 			efi_string_id_t new_id;
>> >-			void *buf;
>> > 			efi_string_t str;
>> > 
>> > 			new_id = ++hii->max_string_id;
>> >-			if (stbl->nstrings < new_id) {
>> >-				buf = realloc(stbl->strings,
>> >-					      sizeof(stbl->strings[0])
>> >-						* new_id);
>> >-				if (!buf)
>> >-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> >-
>> >-				memset(&stbl->strings[stbl->nstrings], 0,
>> >-				       (new_id - stbl->nstrings)
>> >-					 * sizeof(stbl->strings[0]));
>> >-				stbl->strings = buf;
>> >-				stbl->nstrings = new_id;
>> >-			}
>> >+
>> >+			if (!grow_string_table(stbl, new_id))
>> >+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> > 
>> > 			str = u16_strdup(string);
>> > 			if (!str)
>> >@@ -957,20 +977,8 @@ set_string(const struct efi_hii_string_protocol *this,
>> > 			if (hii->max_string_id < string_id)
>> > 				return EFI_EXIT(EFI_NOT_FOUND);
>> > 
>> >-			if (stbl->nstrings < string_id) {
>> >-				void *buf;
>> >-
>> >-				buf = realloc(stbl->strings,
>> >-					      string_id
>> >-						* sizeof(stbl->strings[0]));
>> >-				if (!buf)
>> >-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> >-
>> >-				memset(&stbl->strings[string_id - 1], 0,
>> >-				       (string_id - stbl->nstrings)
>> >-					 * sizeof(stbl->strings[0]));
>> >-				stbl->strings = buf;
>> >-			}
>> >+			if (!grow_string_table(stbl, string_id))
>> >+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> > 
>> > 			str = u16_strdup(string);
>> > 			if (!str)
>> >
>> >---
>> >base-commit: 2c7eaee7b3422ce7c84e892abfe16e1afce42e83
>> >change-id: 20260916-ptr-670be74dcbd3
>> >
>> >Best regards,
>> >--  
>> >Vincent.
>> >
>> 
>

Re: [PATCH] efi_loader: fix string table reallocations

[Vincent Stehlé]

On Fri, Sep 18, 2026 at 06:22:23PM +0200, Heinrich Schuchardt wrote:
> Am 18. September 2026 10:51:42 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
> >On Thu, Sep 17, 2026 at 06:23:11PM +0200, Heinrich Schuchardt wrote:
> >> Am 17. September 2026 11:45:31 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
> >> >The new_string() and set_string() HII string protocol functions keep
> >> >using old pointers after calling realloc(), which may have moved.
> >> >
> >> >The set_string() function also assumes that reallocation only grew the
> >> >array by one element when computing the start of the memory to clear,
> >> >and forgets to update nstrings to the new table size after reallocation.
> >> >
> >> >Repair and factorize the reallocation code in a new grow_string_table()
> >> >function to fix those issues.
> >> 
> >> 
> >> Should we add a test in U-Boot and possibly in SCT?
> >
> >Hi Heinrich,
> >
> >Thanks for having a look.
> >
> >I think the existing "HII database protocols" self-test is sufficient, thanks to
> >the change.
> >
> >With this test, before the patch, the code coverage is fairly good in
> >new_string() but is poor in set_string().
> >
> >After the patch, the code coverage of grow_string_table() is fairly good as
> >expected, and the poor code coverage of set_string() is "gone".
> >
> >Best regards,
> >Vincent.
> 
> If the test did not fail before your code was added, there must be some test case missing.

Hi Heinrich,

Yes, we are indeed missing a test, which would exercise set_string() in
a way that would reallocate the string table, and I did not find an easy
way to obtain that with the existing HII packages data that we have
currently (packagelist1 & 2).

Arguably, now that the factorized function is well covered with
new_string(), we might not even have to do an additional testing effort,
but if you insist I could try to add some more packages data and a
specific unit test.
Just let me know if you prefer that.

Best regards,
Vincent.

> 
> Best regards
> 
> Heinrich
> 
> 
> >
> >> 
> >> Best regards
> >> 
> >> Heinrich
> >> 
> >> >
> >> >Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
> >> >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> >> >Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> >Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> >Cc: Tom Rini <trini@konsulko.com>
> >> >---
> >> >This can be tested on the sandbox with the following command:
> >> >
> >> >  ./u-boot -T -c 'setenv efi_selftest HII database protocols; bootefi selftest'
> >> >
> >> >While the unit test does cause reallocation with new_string(), it does not
> >> >with set_string().
> >> >
> >> >Further testing can be done by forcing stbl->strings = NULL, just after
> >> >calling realloc(stbl->strings, ...).
> >> >---
> >> > lib/efi_loader/efi_hii.c | 64 +++++++++++++++++++++++++++---------------------
> >> > 1 file changed, 36 insertions(+), 28 deletions(-)
> >> >
> >> >diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
> >> >index 7bf51ad43d1..90dff91e5d4 100644
> >> >--- a/lib/efi_loader/efi_hii.c
> >> >+++ b/lib/efi_loader/efi_hii.c
> >> >@@ -822,6 +822,37 @@ static bool language_match(char *language, char *languages)
> >> > 	return false;
> >> > }
> >> > 
> >> >+/**
> >> >+ * grow_string_table() - Grow a string table if necessary.
> >> >+ * @stbl: String table to grow.
> >> >+ * @n:    Minimum table size desired.
> >> >+ *
> >> >+ * Make sure a string table can contain at least n elements.
> >> >+ * If the table is too small, reallocate the memory and clear the new elements.
> >> >+ *
> >> >+ * Return: false in case of reallocation error, true otherwise.
> >> >+ * In case of error, the original string table is left untouched.
> >> >+ * In case of success, stbl->strings and stbl->nstrings may have been updated.
> >> >+ */
> >> >+static bool grow_string_table(struct efi_string_table *stbl, u32 n)
> >> >+{
> >> >+	void *buf;
> >> >+	u32 old_n = stbl->nstrings;
> >> >+
> >> >+	if (old_n >= n)
> >> >+		return true;
> >> >+
> >> >+	buf = realloc(stbl->strings, sizeof(stbl->strings[0]) * n);
> >> >+	if (!buf)
> >> >+		return false;
> >> >+
> >> >+	stbl->strings = buf;
> >> >+	stbl->nstrings = n;
> >> >+	memset(&stbl->strings[old_n], 0, sizeof(stbl->strings[0]) * (n - old_n));
> >> >+
> >> >+	return true;
> >> >+}
> >> >+
> >> > static efi_status_t EFIAPI
> >> > new_string(const struct efi_hii_string_protocol *this,
> >> > 	   efi_hii_handle_t package_list,
> >> >@@ -847,23 +878,12 @@ new_string(const struct efi_hii_string_protocol *this,
> >> > 	list_for_each_entry(stbl, &hii->string_tables, link) {
> >> > 		if (language_match((char *)language, stbl->language)) {
> >> > 			efi_string_id_t new_id;
> >> >-			void *buf;
> >> > 			efi_string_t str;
> >> > 
> >> > 			new_id = ++hii->max_string_id;
> >> >-			if (stbl->nstrings < new_id) {
> >> >-				buf = realloc(stbl->strings,
> >> >-					      sizeof(stbl->strings[0])
> >> >-						* new_id);
> >> >-				if (!buf)
> >> >-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> >> >-
> >> >-				memset(&stbl->strings[stbl->nstrings], 0,
> >> >-				       (new_id - stbl->nstrings)
> >> >-					 * sizeof(stbl->strings[0]));
> >> >-				stbl->strings = buf;
> >> >-				stbl->nstrings = new_id;
> >> >-			}
> >> >+
> >> >+			if (!grow_string_table(stbl, new_id))
> >> >+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> >> > 
> >> > 			str = u16_strdup(string);
> >> > 			if (!str)
> >> >@@ -957,20 +977,8 @@ set_string(const struct efi_hii_string_protocol *this,
> >> > 			if (hii->max_string_id < string_id)
> >> > 				return EFI_EXIT(EFI_NOT_FOUND);
> >> > 
> >> >-			if (stbl->nstrings < string_id) {
> >> >-				void *buf;
> >> >-
> >> >-				buf = realloc(stbl->strings,
> >> >-					      string_id
> >> >-						* sizeof(stbl->strings[0]));
> >> >-				if (!buf)
> >> >-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> >> >-
> >> >-				memset(&stbl->strings[string_id - 1], 0,
> >> >-				       (string_id - stbl->nstrings)
> >> >-					 * sizeof(stbl->strings[0]));
> >> >-				stbl->strings = buf;
> >> >-			}
> >> >+			if (!grow_string_table(stbl, string_id))
> >> >+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> >> > 
> >> > 			str = u16_strdup(string);
> >> > 			if (!str)
> >> >
> >> >---
> >> >base-commit: 2c7eaee7b3422ce7c84e892abfe16e1afce42e83
> >> >change-id: 20260916-ptr-670be74dcbd3
> >> >
> >> >Best regards,
> >> >--  
> >> >Vincent.
> >> >
> >> 
> >
> 

-- 
Vincent.

Re: [PATCH] efi_loader: fix string table reallocations

[Vincent Stehlé]

Hi Heinrich,

The test was simple enough to write after all; I will send a v2.

Best regards,
Vincent.

[PATCH v2 0/2] efi: fix and test hii string table reallocations

[Vincent Stehlé]

Here is a couple of patches to:

- Fix the string table reallocations paths in the HII string protocol
  new_string() and set_string() functions.
- Enhance the HII database protocols selftest to cause reallocation with
  set_string().

Further testing can be done by forcing stbl->strings = NULL, just after
calling realloc(stbl->strings, ...).

Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
---
Changes in v2:
- Enhance the HII unit test to cause reallocation with set_string()
  (suggested by Heinrich).
- Link to v1: https://patch.msgid.link/20260917-ptr-v1-1-7d60d07b849e@arm.com

---
Vincent Stehlé (2):
      efi_loader: fix string table reallocations
      efi_selftest: test set_string() HII string table realloc

 lib/efi_loader/efi_hii.c                 | 64 ++++++++++++++++++--------------
 lib/efi_selftest/efi_selftest_hii.c      | 44 ++++++++++++++++++++++
 lib/efi_selftest/efi_selftest_hii_data.c | 53 ++++++++++++++++++++++++++
 3 files changed, 133 insertions(+), 28 deletions(-)
---
base-commit: a06e89ab05eaa2b8344d521319399333cd760ae5
change-id: 20260916-ptr-670be74dcbd3

Best regards,
--  
Vincent.

[PATCH v2 1/2] efi_loader: fix string table reallocations

[Vincent Stehlé]

The new_string() and set_string() HII string protocol functions keep
using old pointers after calling realloc(), which may have moved.

The set_string() function also assumes that reallocation only grew the
array by one element when computing the start of the memory to clear,
and forgets to update nstrings to the new table size after reallocation.

Repair and factorize the reallocation code in a new grow_string_table()
function to fix those issues.

Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Tom Rini <trini@konsulko.com>
---
 lib/efi_loader/efi_hii.c | 64 +++++++++++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
index 7bf51ad43d1..90dff91e5d4 100644
--- a/lib/efi_loader/efi_hii.c
+++ b/lib/efi_loader/efi_hii.c
@@ -822,6 +822,37 @@ static bool language_match(char *language, char *languages)
 	return false;
 }
 
+/**
+ * grow_string_table() - Grow a string table if necessary.
+ * @stbl: String table to grow.
+ * @n:    Minimum table size desired.
+ *
+ * Make sure a string table can contain at least n elements.
+ * If the table is too small, reallocate the memory and clear the new elements.
+ *
+ * Return: false in case of reallocation error, true otherwise.
+ * In case of error, the original string table is left untouched.
+ * In case of success, stbl->strings and stbl->nstrings may have been updated.
+ */
+static bool grow_string_table(struct efi_string_table *stbl, u32 n)
+{
+	void *buf;
+	u32 old_n = stbl->nstrings;
+
+	if (old_n >= n)
+		return true;
+
+	buf = realloc(stbl->strings, sizeof(stbl->strings[0]) * n);
+	if (!buf)
+		return false;
+
+	stbl->strings = buf;
+	stbl->nstrings = n;
+	memset(&stbl->strings[old_n], 0, sizeof(stbl->strings[0]) * (n - old_n));
+
+	return true;
+}
+
 static efi_status_t EFIAPI
 new_string(const struct efi_hii_string_protocol *this,
 	   efi_hii_handle_t package_list,
@@ -847,23 +878,12 @@ new_string(const struct efi_hii_string_protocol *this,
 	list_for_each_entry(stbl, &hii->string_tables, link) {
 		if (language_match((char *)language, stbl->language)) {
 			efi_string_id_t new_id;
-			void *buf;
 			efi_string_t str;
 
 			new_id = ++hii->max_string_id;
-			if (stbl->nstrings < new_id) {
-				buf = realloc(stbl->strings,
-					      sizeof(stbl->strings[0])
-						* new_id);
-				if (!buf)
-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
-
-				memset(&stbl->strings[stbl->nstrings], 0,
-				       (new_id - stbl->nstrings)
-					 * sizeof(stbl->strings[0]));
-				stbl->strings = buf;
-				stbl->nstrings = new_id;
-			}
+
+			if (!grow_string_table(stbl, new_id))
+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
 
 			str = u16_strdup(string);
 			if (!str)
@@ -957,20 +977,8 @@ set_string(const struct efi_hii_string_protocol *this,
 			if (hii->max_string_id < string_id)
 				return EFI_EXIT(EFI_NOT_FOUND);
 
-			if (stbl->nstrings < string_id) {
-				void *buf;
-
-				buf = realloc(stbl->strings,
-					      string_id
-						* sizeof(stbl->strings[0]));
-				if (!buf)
-					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
-
-				memset(&stbl->strings[string_id - 1], 0,
-				       (string_id - stbl->nstrings)
-					 * sizeof(stbl->strings[0]));
-				stbl->strings = buf;
-			}
+			if (!grow_string_table(stbl, string_id))
+				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
 
 			str = u16_strdup(string);
 			if (!str)

-- 
2.53.0

[PATCH v2 2/2] efi_selftest: test set_string() HII string table realloc

[Vincent Stehlé]

Enhance the HII selftest to cause string table reallocation with
set_string().

The test method is the following:
1. Add string packages with two strings for en-US, and only one string
   for fr-FR.
2. Set string 2 for fr-FR, causing reallocation of the string table.
3. Get string 2 again, to verify that we did not forget the new string
   table size.

This can be tested on the sandbox with the following command:

  ./u-boot -T -c "setenv efi_selftest HII database protocols; \
                  bootefi selftest"

Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Tom Rini <trini@konsulko.com>
---
 lib/efi_selftest/efi_selftest_hii.c      | 44 ++++++++++++++++++++++++++
 lib/efi_selftest/efi_selftest_hii_data.c | 53 ++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)

diff --git a/lib/efi_selftest/efi_selftest_hii.c b/lib/efi_selftest/efi_selftest_hii.c
index fdbb08fb417..2d7151b638f 100644
--- a/lib/efi_selftest/efi_selftest_hii.c
+++ b/lib/efi_selftest/efi_selftest_hii.c
@@ -820,6 +820,8 @@ static int test_hii_string_set_string(void)
 	efi_string_id_t id;
 	efi_status_t ret;
 	int result = EFI_ST_FAILURE;
+	u16 string[32];
+	efi_uintn_t string_size;
 
 	PRINT_TESTNAME;
 	ret = hii_database_protocol->new_package_list(hii_database_protocol,
@@ -849,6 +851,48 @@ static int test_hii_string_set_string(void)
 		goto out;
 	}
 
+	ret = hii_database_protocol->remove_package_list(hii_database_protocol,
+							 handle);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("remove_package_list returned %u\n",
+			     (unsigned int)ret);
+		return EFI_ST_FAILURE;
+	}
+
+	/*
+	 * Test the reallocation path.
+	 * We create string 2 in package fr-FR, which has only one string.
+	 * This forces the reallocation of the string table.
+	 * Then we read string 2 again, to verify that we did not forget the
+	 * size of the string table.
+	 */
+	ret = hii_database_protocol->new_package_list(hii_database_protocol,
+			(struct efi_hii_package_list_header *)packagelist3,
+			NULL, &handle);
+	if (ret != EFI_SUCCESS || !handle) {
+		efi_st_error("new_package_list returned %u\n",
+			     (unsigned int)ret);
+		return EFI_ST_FAILURE;
+	}
+
+	ret = hii_string_protocol->set_string(hii_string_protocol, handle, 2,
+					      (u8 *)"fr-FR", u"Realloc", NULL);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("set_string returned %u\n",
+			     (unsigned int)ret);
+		goto out;
+	}
+
+	string_size = sizeof(string);
+	ret = hii_string_protocol->get_string(hii_string_protocol,
+					      (u8 *)"fr-FR", handle, 2, string,
+					      &string_size, NULL);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("get_string returned %u\n",
+			     (unsigned int)ret);
+		goto out;
+	}
+
 	result = EFI_ST_SUCCESS;
 
 out:
diff --git a/lib/efi_selftest/efi_selftest_hii_data.c b/lib/efi_selftest/efi_selftest_hii_data.c
index 2d29bb045d3..7ab70c61148 100644
--- a/lib/efi_selftest/efi_selftest_hii_data.c
+++ b/lib/efi_selftest/efi_selftest_hii_data.c
@@ -424,6 +424,59 @@ static u8 packagelist2[] = {
 	EFI_HII_PACKAGE_END			// 1
 };
 
+/*
+ * This package list is meant for testing the reallocation path of set_string().
+ * It contains two strings in en-US and only one string in fr-FR.
+ */
+static u8 packagelist3[] = {
+	// EFI_HII_PACKAGE_LIST_HEADER, length = 20
+	0xd3, 0xde, 0x85, 0x86, 0xce, 0x1b, 0xf3, 0x43,
+	0xa2, 0x0c, 0xa3, 0x06, 0xec, 0x69, 0x72, 0xdd,	//16
+	0xad, 0x00, 0x00, 0x00,				// 4: total 173
+	//
+	// EFI_HII_PACKAGE_STRINGS, length = 77
+	//
+	0x4d, 0x00, 0x00,				// 3: length(header)
+	EFI_HII_PACKAGE_STRINGS,			// 1: type(header)
+	0x34, 0x00, 0x00, 0x00,				// 4: header_size
+	0x34, 0x00, 0x00, 0x00,				// 4: string_offset
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,	//32: language_window
+	0x01, 0x00,					// 2: language name
+	0x65, 0x6e, 0x2d, 0x55, 0x53, 0x00,		// 6: language "en-US"
+	EFI_HII_SIBT_STRING_UCS2,			// 1
+	0x45, 0x00, 0x6E, 0x00, 0x67, 0x00, 0x6C, 0x00,
+	0x69, 0x00, 0x73, 0x00, 0x68, 0x00, 0x00, 0x00,	//16: "English"
+	EFI_HII_SIBT_STRING_UCS2,			// 1
+	0x55, 0x00, 0x53, 0x00, 0x00, 0x00,		// 6: "US"
+	EFI_HII_SIBT_END,				// 1
+	//
+	// EFI_HII_PACKAGE_STRINGS, length = 72
+	//
+	0x48, 0x00, 0x00,				// 3: length(header)
+	EFI_HII_PACKAGE_STRINGS,			// 1: type(header)
+	0x34, 0x00, 0x00, 0x00,				// 4: header_size
+	0x34, 0x00, 0x00, 0x00,				// 4: string_offset
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,	//32: language_window
+	0x01, 0x00,					// 2: language name
+	0x66, 0x72, 0x2d, 0x46, 0x52, 0x00,		// 6: language "fr-FR"
+	EFI_HII_SIBT_STRING_UCS2,			// 1
+	0x46, 0x00, 0x72, 0x00, 0x61, 0x00, 0x6e, 0x00,
+	0xe7, 0x00, 0x61, 0x00, 0x69, 0x00, 0x73, 0x00,
+	0x00, 0x00,					//18: "Français"
+	EFI_HII_SIBT_END,				// 1
+	//
+	// End of package list, length = 4
+	//
+	0x4, 0x00, 0x00,				// 3
+	EFI_HII_PACKAGE_END				// 1
+};
+
 static efi_guid_t kb_layout_guid11 =
 	EFI_GUID(0x8d40e495, 0xe2aa, 0x4c6f,
 		 0x89, 0x70, 0x68, 0x85, 0x09, 0xee, 0xc7, 0xd2);

-- 
2.53.0

Re: [PATCH v2 1/2] efi_loader: fix string table reallocations

[Heinrich Schuchardt]

On 9/22/26 11:15, Vincent Stehlé wrote:
> The new_string() and set_string() HII string protocol functions keep
> using old pointers after calling realloc(), which may have moved.
> 
> The set_string() function also assumes that reallocation only grew the
> array by one element when computing the start of the memory to clear,
> and forgets to update nstrings to the new table size after reallocation.
> 
> Repair and factorize the reallocation code in a new grow_string_table()
> function to fix those issues.
> 
> Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Cc: Tom Rini <trini@konsulko.com>

LGTM

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> ---
>   lib/efi_loader/efi_hii.c | 64 +++++++++++++++++++++++++++---------------------
>   1 file changed, 36 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
> index 7bf51ad43d1..90dff91e5d4 100644
> --- a/lib/efi_loader/efi_hii.c
> +++ b/lib/efi_loader/efi_hii.c
> @@ -822,6 +822,37 @@ static bool language_match(char *language, char *languages)
>   	return false;
>   }
>   
> +/**
> + * grow_string_table() - Grow a string table if necessary.
> + * @stbl: String table to grow.
> + * @n:    Minimum table size desired.
> + *
> + * Make sure a string table can contain at least n elements.
> + * If the table is too small, reallocate the memory and clear the new elements.
> + *
> + * Return: false in case of reallocation error, true otherwise.
> + * In case of error, the original string table is left untouched.
> + * In case of success, stbl->strings and stbl->nstrings may have been updated.
> + */
> +{
> +	void *buf;
> +	u32 old_n = stbl->nstrings;
> +
> +	if (old_n >= n)
> +		return true;
> +
> +	buf = realloc(stbl->strings, sizeof(stbl->strings[0]) * n);
> +	if (!buf)
> +		return false;
> +
> +	stbl->strings = buf;
> +	stbl->nstrings = n;
> +	memset(&stbl->strings[old_n], 0, sizeof(stbl->strings[0]) * (n - old_n));
> +
> +	return true;
> +}
> +
>   static efi_status_t EFIAPI
>   new_string(const struct efi_hii_string_protocol *this,
>   	   efi_hii_handle_t package_list,
> @@ -847,23 +878,12 @@ new_string(const struct efi_hii_string_protocol *this,
>   	list_for_each_entry(stbl, &hii->string_tables, link) {
>   		if (language_match((char *)language, stbl->language)) {
>   			efi_string_id_t new_id;
> -			void *buf;
>   			efi_string_t str;
>   
>   			new_id = ++hii->max_string_id;
> -			if (stbl->nstrings < new_id) {
> -				buf = realloc(stbl->strings,
> -					      sizeof(stbl->strings[0])
> -						* new_id);
> -				if (!buf)
> -					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> -
> -				memset(&stbl->strings[stbl->nstrings], 0,
> -				       (new_id - stbl->nstrings)
> -					 * sizeof(stbl->strings[0]));
> -				stbl->strings = buf;
> -				stbl->nstrings = new_id;
> -			}
> +
> +			if (!grow_string_table(stbl, new_id))
> +				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>   
>   			str = u16_strdup(string);
>   			if (!str)
> @@ -957,20 +977,8 @@ set_string(const struct efi_hii_string_protocol *this,
>   			if (hii->max_string_id < string_id)
>   				return EFI_EXIT(EFI_NOT_FOUND);
>   
> -			if (stbl->nstrings < string_id) {
> -				void *buf;
> -
> -				buf = realloc(stbl->strings,
> -					      string_id
> -						* sizeof(stbl->strings[0]));
> -				if (!buf)
> -					return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> -
> -				memset(&stbl->strings[string_id - 1], 0,
> -				       (string_id - stbl->nstrings)
> -					 * sizeof(stbl->strings[0]));
> -				stbl->strings = buf;
> -			}
> +			if (!grow_string_table(stbl, string_id))
> +				return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>   
>   			str = u16_strdup(string);
>   			if (!str)
>