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

On Mon, Sep 27, 2021 at 5:46 PM Philipp Rudo <prudo(a)redhat.com&gt; wrote: > > Hi Lianbo, > > On Mon, 27 Sep 2021 11:33:31 +0800 > lijiang <lijiang(a)redhat.com&gt; wrote: > > > > Date: Sat, 18 Sep 2021 15:59:30 +0800 > > > From: Tao Liu <ltao(a)redhat.com&gt; > > > To: crash-utility(a)redhat.com > > > Subject: [Crash-utility] [PATCH v4 2/4] Get the absolute value of > > > SYMNAME_HASH_INDEX > > > Message-ID: <20210918075932.132339-3-ltao(a)redhat.com&gt; > > > 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(a)redhat.com&gt; > > > Reviewed-by: Philipp Rudo <prudo(a)redhat.com&gt; > > > --- > > > 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(a)redhat.com > > https://listman.redhat.com/mailman/listinfo/crash-utility > > >