Hi Nicolas,
Thank you for the patch.
On mer., déc. 11, 2024 at 14:53, Nicolas Belin nbelin@baylibre.com wrote:
Rework the bootargs concatenation allocating more accurately the length that is needed. Do not forget an extra byte for the null termination byte as, in some cases, the allocation was 1 byte short.
Fixes: 86f4695b ("image: Fix Android boot image support") Signed-off-by: Nicolas Belin nbelin@baylibre.com
boot/image-android.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/boot/image-android.c b/boot/image-android.c index 362a5c7435a3a8bcf7b674b96e31069a91a892b5..ed72a5c30424a453c1800bc61edbe8f33b31b341 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -289,7 +289,7 @@ int android_image_get_kernel(const void *hdr, int len = 0; if (*img_data.kcmdline) { printf("Kernel command line: %s\n", img_data.kcmdline);
len += strlen(img_data.kcmdline);
len += strlen(img_data.kcmdline) + 1; /* Extra space character needed */}
if (*img_data.kcmdline_extra) {
@@ -299,28 +299,28 @@ int android_image_get_kernel(const void *hdr,
char *bootargs = env_get("bootargs"); if (bootargs)
len += strlen(bootargs);
len += strlen(bootargs) + 1; /* Extra space character needed */
In some cases, we will allocate for an extra space character when this is not needed.
For example, with: * bootargs = "b" * kcmdline = NULL * kcmdline_extra = NULL
We will allocate strlen("b") + 1 + 1. But we only need to allocate strlen("b") + 1.
Another example: * bootargs = "b" * kcmdline = "k" * kcmdline_extra = NULL
Will allocate strlen("b") + 1 + strlen("k") + 1 + 1. But we only need: strlen("b") + 1 + strlen("k") + 1.
How about we count the amount of elements that are not NULL to compute the amount of spaces: * 0 -> 0 space * 1 -> 0 space * 2 -> 1 space * 3 -> 2 spaces
- char *newbootargs = malloc(len + 2);
- char *newbootargs = malloc(len + 1); /* +1 for the '\0' */ if (!newbootargs) { puts("Error: malloc in android_image_get_kernel failed!\n"); return -ENOMEM; }
- *newbootargs = '\0';
- *newbootargs = '\0'; /* set to NULL in case no components below are present */
The comment should state Null, not NULL. NULL is the pointer and Null is the \0 character.
if (bootargs) { strcpy(newbootargs, bootargs); strcat(newbootargs, " ");
A similar thing can be stated here. Sometimes, we add spaces which are not needed.
Can we rework this a little to only add what is needed?
}
- if (*img_data.kcmdline)
- if (*img_data.kcmdline) { strcat(newbootargs, img_data.kcmdline);
- if (*img_data.kcmdline_extra) { strcat(newbootargs, " ");
strcat(newbootargs, img_data.kcmdline_extra);
- if (*img_data.kcmdline_extra)
strcat(newbootargs, img_data.kcmdline_extra);- env_set("bootargs", newbootargs); free(newbootargs);
-- 2.34.1