[PATCH 0/4] boot: android: rework the bootargs concatenation

[PATCH 0/4] boot: android: rework the bootargs concatenation

[Nicolas Belin]

Rework the bootargs concatenation logic to make the logic clearer
and address several issues such as:
- checking the value at the address kcmdline_extra instead of
checking the address value itself
- freeing the string that was malloced for the concatenation
- making sure to malloc the right amount of bytes for the concatenated
string, not forgetting the byte for the NULL termination 

Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
---
Nicolas Belin (4):
      boot: android: fix extra command line support
      boot: android: free newbootargs when done
      boot: android: rework bootargs concatenation
      boot: android: reorder the length calculation order

 boot/image-android.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)
---
base-commit: b841e559cd26ffcb20f22e8ee75debed9616c002
change-id: 20241211-fix-bootargs-concatenation-8b616c110b63

Best regards,
-- 
Nicolas Belin <nbelin@baylibre.com>

[PATCH 1/4] boot: android: fix extra command line support

[Nicolas Belin]

Check that the value at the address kcmdline_extra is not 0
instead of checking the address value itself keeping it
consistent with what is done for kcmdline.

Fixes: b36b227b ("android: boot: support extra command line")
Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
---
 boot/image-android.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/boot/image-android.c b/boot/image-android.c
index cd01278f211d63262f2bdad7aa1176e2c1bbfedd..57158280b41c6552c82838e21384d925d5f7cde4 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -292,7 +292,7 @@ int android_image_get_kernel(const void *hdr,
 		len += strlen(img_data.kcmdline);
 	}
 
-	if (img_data.kcmdline_extra) {
+	if (*img_data.kcmdline_extra) {
 		printf("Kernel extra command line: %s\n", img_data.kcmdline_extra);
 		len += strlen(img_data.kcmdline_extra);
 	}
@@ -316,7 +316,7 @@ int android_image_get_kernel(const void *hdr,
 	if (*img_data.kcmdline)
 		strcat(newbootargs, img_data.kcmdline);
 
-	if (img_data.kcmdline_extra) {
+	if (*img_data.kcmdline_extra) {
 		strcat(newbootargs, " ");
 		strcat(newbootargs, img_data.kcmdline_extra);
 	}

-- 
2.34.1

[PATCH 2/4] boot: android: free newbootargs when done

[Nicolas Belin]

Free newbootargs when the concatenation is done and bootargs env
is set.

Fixes: 86f4695b ("image: Fix Android boot image support")
Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
---
 boot/image-android.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/boot/image-android.c b/boot/image-android.c
index 57158280b41c6552c82838e21384d925d5f7cde4..362a5c7435a3a8bcf7b674b96e31069a91a892b5 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -322,6 +322,7 @@ int android_image_get_kernel(const void *hdr,
 	}
 
 	env_set("bootargs", newbootargs);
+	free(newbootargs);
 
 	if (os_data) {
 		if (image_get_magic(ihdr) == IH_MAGIC) {

-- 
2.34.1

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

[Nicolas Belin]

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 */
 
-	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 */
 
 	if (bootargs) {
 		strcpy(newbootargs, bootargs);
 		strcat(newbootargs, " ");
 	}
 
-	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

[PATCH 4/4] boot: android: reorder the length calculation order

[Nicolas Belin]

Keep the same order (bootargs then kcmdline then kcmdline_extra)
for both the length calculation and the string concatenation steps.

Signed-off-by: Nicolas Belin <nbelin@baylibre.com>
---
 boot/image-android.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/boot/image-android.c b/boot/image-android.c
index ed72a5c30424a453c1800bc61edbe8f33b31b341..a76f028090d0470842bbf559b638175949c527f0 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -287,6 +287,11 @@ int android_image_get_kernel(const void *hdr,
 	       kernel_addr, DIV_ROUND_UP(img_data.kernel_size, 1024));
 
 	int len = 0;
+	char *bootargs = env_get("bootargs");
+
+	if (bootargs)
+		len += strlen(bootargs) + 1; /* Extra space character needed */
+
 	if (*img_data.kcmdline) {
 		printf("Kernel command line: %s\n", img_data.kcmdline);
 		len += strlen(img_data.kcmdline) + 1; /* Extra space character needed */
@@ -297,10 +302,6 @@ int android_image_get_kernel(const void *hdr,
 		len += strlen(img_data.kcmdline_extra);
 	}
 
-	char *bootargs = env_get("bootargs");
-	if (bootargs)
-		len += strlen(bootargs) + 1; /* Extra space character needed */
-
 	char *newbootargs = malloc(len + 1); /* +1 for the '\0' */
 	if (!newbootargs) {
 		puts("Error: malloc in android_image_get_kernel failed!\n");

-- 
2.34.1

Re: [PATCH 1/4] boot: android: fix extra command line support

[Mattijs Korpershoek]

Hi Nicolas,

Thank you for the patch.

On mer., déc. 11, 2024 at 14:53, Nicolas Belin <nbelin@baylibre.com> wrote:

> Check that the value at the address kcmdline_extra is not 0
> instead of checking the address value itself keeping it
> consistent with what is done for kcmdline.
>
> Fixes: b36b227b ("android: boot: support extra command line")
> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  boot/image-android.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/boot/image-android.c b/boot/image-android.c
> index cd01278f211d63262f2bdad7aa1176e2c1bbfedd..57158280b41c6552c82838e21384d925d5f7cde4 100644
> --- a/boot/image-android.c
> +++ b/boot/image-android.c
> @@ -292,7 +292,7 @@ int android_image_get_kernel(const void *hdr,
>  		len += strlen(img_data.kcmdline);
>  	}
>  
> -	if (img_data.kcmdline_extra) {
> +	if (*img_data.kcmdline_extra) {
>  		printf("Kernel extra command line: %s\n", img_data.kcmdline_extra);
>  		len += strlen(img_data.kcmdline_extra);
>  	}
> @@ -316,7 +316,7 @@ int android_image_get_kernel(const void *hdr,
>  	if (*img_data.kcmdline)
>  		strcat(newbootargs, img_data.kcmdline);
>  
> -	if (img_data.kcmdline_extra) {
> +	if (*img_data.kcmdline_extra) {
>  		strcat(newbootargs, " ");
>  		strcat(newbootargs, img_data.kcmdline_extra);
>  	}
>
> -- 
> 2.34.1

Re: [PATCH 2/4] boot: android: free newbootargs when done

[Mattijs Korpershoek]

Hi Nicolas,

Thank you for the patch.

On mer., déc. 11, 2024 at 14:53, Nicolas Belin <nbelin@baylibre.com> wrote:

> Free newbootargs when the concatenation is done and bootargs env
> is set.
>
> Fixes: 86f4695b ("image: Fix Android boot image support")
> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  boot/image-android.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/boot/image-android.c b/boot/image-android.c
> index 57158280b41c6552c82838e21384d925d5f7cde4..362a5c7435a3a8bcf7b674b96e31069a91a892b5 100644
> --- a/boot/image-android.c
> +++ b/boot/image-android.c
> @@ -322,6 +322,7 @@ int android_image_get_kernel(const void *hdr,
>  	}
>  
>  	env_set("bootargs", newbootargs);
> +	free(newbootargs);
>  
>  	if (os_data) {
>  		if (image_get_magic(ihdr) == IH_MAGIC) {
>
> -- 
> 2.34.1

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

[Mattijs Korpershoek]

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

Re: [PATCH 4/4] boot: android: reorder the length calculation order

[Mattijs Korpershoek]

Hi Nicolas,

Thank you for the patch.

On mer., déc. 11, 2024 at 14:53, Nicolas Belin <nbelin@baylibre.com> wrote:

> Keep the same order (bootargs then kcmdline then kcmdline_extra)
> for both the length calculation and the string concatenation steps.
>
> Signed-off-by: Nicolas Belin <nbelin@baylibre.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> ---
>  boot/image-android.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/boot/image-android.c b/boot/image-android.c
> index ed72a5c30424a453c1800bc61edbe8f33b31b341..a76f028090d0470842bbf559b638175949c527f0 100644
> --- a/boot/image-android.c
> +++ b/boot/image-android.c
> @@ -287,6 +287,11 @@ int android_image_get_kernel(const void *hdr,
>  	       kernel_addr, DIV_ROUND_UP(img_data.kernel_size, 1024));
>  
>  	int len = 0;
> +	char *bootargs = env_get("bootargs");
> +
> +	if (bootargs)
> +		len += strlen(bootargs) + 1; /* Extra space character needed */
> +
>  	if (*img_data.kcmdline) {
>  		printf("Kernel command line: %s\n", img_data.kcmdline);
>  		len += strlen(img_data.kcmdline) + 1; /* Extra space character needed */
> @@ -297,10 +302,6 @@ int android_image_get_kernel(const void *hdr,
>  		len += strlen(img_data.kcmdline_extra);
>  	}
>  
> -	char *bootargs = env_get("bootargs");
> -	if (bootargs)
> -		len += strlen(bootargs) + 1; /* Extra space character needed */
> -
>  	char *newbootargs = malloc(len + 1); /* +1 for the '\0' */
>  	if (!newbootargs) {
>  		puts("Error: malloc in android_image_get_kernel failed!\n");
>
> -- 
> 2.34.1

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

[Nicolas Belin]

Le lun. 16 déc. 2024 à 09:41, Mattijs Korpershoek
<mkorpershoek@baylibre.com> a écrit :
>
> 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?

Will do for v2

>
> >       }
> >
> > -     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