Hi Nicolas, Thank you for the patch.
On mer., déc. 11, 2024 at 14:53, Nicolas Belin <[email protected]> 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 <[email protected]> > --- > 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
