Re: [PATCH 3/4] boot: android: rework bootargs concatenation

Mattijs Korpershoek
2024-12-16T08:41:14Z

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