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

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