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

[Nicolas Belin]

Le lun. 16 déc. 2024 à 09:41, Mattijs Korpershoek
<mkorpershoek at baylibre.com> a écrit :
>
> Hi Nicolas,
>
> Thank you for the patch.
>
> On mer., déc. 11, 2024 at 14:53, Nicolas Belin <nbelin at 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 at 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