[Crash-utility] [PATCH v4 2/4] Get the absolute value of SYMNAME_HASH_INDEX

[Tao Liu]

Hello lijiang and Philipp,

On Mon, Sep 27, 2021 at 8:00 PM Philipp Rudo <prudo at redhat.com> wrote:
>
> Hi Lianbo,
>
> On Mon, 27 Sep 2021 19:05:24 +0800
> lijiang <lijiang at redhat.com> wrote:
>
> > On Mon, Sep 27, 2021 at 5:46 PM Philipp Rudo <prudo at redhat.com> wrote:
> > >
> > > Hi Lianbo,
> > >
> > > On Mon, 27 Sep 2021 11:33:31 +0800
> > > lijiang <lijiang at redhat.com> wrote:
> > >
> > > > > Date: Sat, 18 Sep 2021 15:59:30 +0800
> > > > > From: Tao Liu <ltao at redhat.com>
> > > > > To: crash-utility at redhat.com
> > > > > Subject: [Crash-utility] [PATCH v4 2/4] Get the absolute value of
> > > > >         SYMNAME_HASH_INDEX
> > > > > Message-ID: <20210918075932.132339-3-ltao at redhat.com>
> > > > > Content-Type: text/plain; charset="US-ASCII"
> > > > >
> > > > > SYMNAME_HASH_INDEX is used as the index of symname hash table. It will
> > > > > be out of range if SYMNAME_HASH_INDEX is negative. Let's get its absolute
> > > > > value to avoid such risk.
> > > > >
> > > > > Signed-off-by: Tao Liu <ltao at redhat.com>
> > > > > Reviewed-by: Philipp Rudo <prudo at redhat.com>
> > > > > ---
> > > > >  defs.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/defs.h b/defs.h
> > > > > index c10ebff..3129f06 100644
> > > > > --- a/defs.h
> > > > > +++ b/defs.h
> > > > > @@ -2725,7 +2725,7 @@ struct downsized {
> > > > >
> > > > >  #define SYMNAME_HASH (512)
> > > > >  #define SYMNAME_HASH_INDEX(name) \
> > > > > - ((name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2])) % SYMNAME_HASH)
> > > > > + (abs((name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2])) % SYMNAME_HASH))
> > > > >
> > > >
> > > > I guess this may be caused by integer overflow, the expression uses
> > > > integer calculations by default.  Does the following change work for
> > > > you?
> > > >
> > > > -#define SYMNAME_HASH (512)
> > > > +#define SYMNAME_HASH (512ULL)
> > >
> > > the above isn't sufficient. The sign of the result is defined by the
> > > sign of the dividend, i.e. assume a % b, then the result has the same
> > > sign as a. So the result of SYMNAME_HASH_INDEX will become negative
> > > whenever
> > >
> > > name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2])
> > >
> > > is negative. So the only alternative I see is to guarantee that 'name'
> > > is an unsigned char. But that would be more complicated to implement
> > > then simply adding the 'abs'.
> > >
> >
> > If it still doesn't work, I would tend to remove the macro
> > definition(SYMNAME_HASH_INDEX)
> > from defs.h, and change the macro definition to a static function in
> > the symbols.c, let's use the
> > 'unsigned long long' variable to calculate it.
> >
> > Currently, the macro is used twice in the symbols.c. This change seems
> > not complicated. Any thoughts?
>
> do I understand your suggestion correct, you propose to replace the
>
>         #define SYMNAME_HASH_INDEX(name) ...
>
> in defs.h by something like
>
>         static unsigned long long SYMNAME_HASH_INDEX(const unsigned char * const name) {
>                 return (name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2]) % SYMNAME_HASH);
>         }
>
> in symbols.c? If so, I think that should be fine.
>

Please correct me if I'm wrong. I don't think the function can work.
Let's say name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2]) ==
-1, then we will have:

static unsigned long long SYMNAME_HASH_INDEX(const unsigned char * const name) {
        return (-1) % 512;
}

The returned value is a very large number, and will overflow the array.

Thanks,
Tao Liu

> Thanks
> Philipp
>
>
> > Thanks.
> > Lianbo
> >
> >
> > > Thanks
> > > Philipp
> > >
> > > >
> > > > Thanks.
> > > > Lianbo
> > > > >  #define PATCH_KERNEL_SYMBOLS_START  ((char *)(1))
> > > > >  #define PATCH_KERNEL_SYMBOLS_STOP   ((char *)(2))
> > > > > --
> > > > > 2.29.2
> > > > >
> > > >
> > > > --
> > > > Crash-utility mailing list
> > > > Crash-utility at redhat.com
> > > > https://listman.redhat.com/mailman/listinfo/crash-utility
> > > >
> > >
> >
>
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://listman.redhat.com/mailman/listinfo/crash-utility
>