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