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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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