[PATCH v4 19/23] cmd: Fix memory-mapping in cmp command

[Simon Glass]

Hi Quentin,

On Tue, 3 Sept 2024 at 11:42, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Simon,
>
> On 9/2/24 12:26 AM, Simon Glass wrote:
> > This unmaps a different address from what was mapped. Fix it.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > (no changes since v1)
> >
> >   cmd/mem.c | 26 +++++++++++++-------------
> >   1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/cmd/mem.c b/cmd/mem.c
> > index 274348068c2..4d6fde28531 100644
> > --- a/cmd/mem.c
> > +++ b/cmd/mem.c
> > @@ -245,7 +245,7 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc,
> >       int     size;
> >       int     rcode = 0;
> >       const char *type;
> > -     const void *buf1, *buf2, *base;
> > +     const void *buf1, *buf2, *base, *ptr1, *ptr2;
> >       ulong word1, word2;  /* 64-bit if MEM_SUPPORT_64BIT_DATA */
> >
> >       if (argc != 4)
> > @@ -270,22 +270,22 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc,
> >       bytes = size * count;
> >       base = buf1 = map_sysmem(addr1, bytes);
>
> "base" isn't changed in the rest of the code, so we could just reuse it
> instead of declaring yet another variable.
>
> >       buf2 = map_sysmem(addr2, bytes);
>
> We could also set ptr2 here... Allowing to avoid the diff from here to....
>
> > -     for (ngood = 0; ngood < count; ++ngood) {
> > +     for (ngood = 0, ptr1 = buf1, ptr2 = buf2; ngood < count; ++ngood) { >                   if (size == 4) {
> > -                     word1 = *(u32 *)buf1;
> > -                     word2 = *(u32 *)buf2;
> > +                     word1 = *(u32 *)ptr1;
> > +                     word2 = *(u32 *)ptr2;
> >               } else if (MEM_SUPPORT_64BIT_DATA && size == 8) {
> > -                     word1 = *(ulong *)buf1;
> > -                     word2 = *(ulong *)buf2;
> > +                     word1 = *(ulong *)ptr1;
> > +                     word2 = *(ulong *)ptr2;
> >               } else if (size == 2) {
> > -                     word1 = *(u16 *)buf1;
> > -                     word2 = *(u16 *)buf2;
> > +                     word1 = *(u16 *)ptr1;
> > +                     word2 = *(u16 *)ptr2;
> >               } else {
> > -                     word1 = *(u8 *)buf1;
> > -                     word2 = *(u8 *)buf2;
> > +                     word1 = *(u8 *)ptr1;
> > +                     word2 = *(u8 *)ptr2;
> >               }
> >               if (word1 != word2) {
> > -                     ulong offset = buf1 - base;
> > +                     ulong offset = ptr1 - base;
> >                       printf("%s at 0x%08lx (%#0*lx) != %s at 0x%08lx (%#0*lx)\n",
> >                               type, (ulong)(addr1 + offset), size, word1,
> >                               type, (ulong)(addr2 + offset), size, word2);
> > @@ -293,8 +293,8 @@ static int do_mem_cmp(struct cmd_tbl *cmdtp, int flag, int argc,
> >                       break;
> >               }
> >
> > -             buf1 += size;
> > -             buf2 += size;
> > +             ptr1 += size;
> > +             ptr2 += size;
> >
>
> here - making the commit all the more explicit (for me this commit is
> basically only renaming a variable, since unmap_system doesn't appear in
> the git context) - by only changing:
>
> unmap_system(buf1);
> unmap_system(buf2);
>
> to
>
> unmap_system(base);
> unmap_system(ptr2);
>
> I believe?
>
> Additionally, my linter tells me that:
>
> buf1 += size;
> buf2 += size;
>
> is undefined behavior:
>
> arithOperationsOnVoidPointer: 'buf1' is of type 'const void *'. When
> using void pointers in calculations, the behaviour is undefined.

Hmmm I thought that got changed...or at least I rely on void * being
taken as char * when diffing pointers.

>
> I suggest the following:
>
> buf1 = ((u8 *)buf1) + size;
> buf2 = ((u8 *)buf2) + size;
>
> since size seems to be size in bytes?
>
> What do you think?

Well I think you should send a patch! It seems reasonable to me.

>
> We already have test/cmd/mem.c is this something we can augment to test
> the unmapping is proper too?

Yes, it would be possible to check the list of mapped pointers in
sandbox, to make sure it has gone back to its original state (i.e. any
new mappings in the test were deleted before the test ended).

Regards,
Simon