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

[lijiang]

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?

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
> >
>