Re: Avoid overflow with simplehash - Mailing list pgsql-hackers

From Ranier Vilela
Subject Re: Avoid overflow with simplehash
Date
Msg-id CAEudQAo3JORGkf3XvvYm2gwbrSGhDELrg+aRSqGqb2TZxK6njQ@mail.gmail.com
Whole thread Raw
In response to Re: Avoid overflow with simplehash  (Andres Freund <andres@anarazel.de>)
Responses Re: Avoid overflow with simplehash
List pgsql-hackers


Em qui., 6 de jul. de 2023 às 13:52, Andres Freund <andres@anarazel.de> escreveu:
Hi,

On 2023-07-06 13:40:00 -0300, Ranier Vilela wrote:
> I still have doubts about this.
>
> see:
> #include <iostream>
> #include <string>
> #include <limits.h>
>
> #define SH_MAX_SIZE1 (((unsigned long long) 0xFFFFFFFFU) + 1)
> #define SH_MAX_SIZE2 (((unsigned long long) 0xFFFFFFFFU) - 1)
>
> int main()
> {
>     unsigned long long max_size1 = SH_MAX_SIZE1;
>     unsigned long long max_size2 = SH_MAX_SIZE2;
>     unsigned int cur1 = SH_MAX_SIZE1;
>     unsigned int cur2 = SH_MAX_SIZE2;
>
>     printf("SH_MAX_SIZE1=%llu\n", max_size1);
>     printf("SH_MAX_SIZE2=%llu\n", max_size2);
>     printf("cur1=%u\n", cur1);
>     printf("cur2=%u\n", cur2);
> }
> warning: implicit conversion from 'unsigned long long' to 'unsigned int'
> changes value from 4294967296 to 0 [-Wconstant-conversion]

I don't think we at the moment try to not have implicit-conversion warnings
(nor do we enable them), this would be far from the only place. If we wanted
to here, we'd just need an explicit cast.
It was just for show.
 


> outputs:
> SH_MAX_SIZE1=4294967296
> SH_MAX_SIZE2=4294967294
> cur1=0
> cur2=4294967294
>
> And in the comments we have:
> "Iterate backwards, that allows the current element to be deleted, even
> * if there are backward shifts"
>
> So if an empty element is not found and the *cur* field is set to 0
> (SH_MAX_SIZE -> uint32),

That should never be reachable - which the assert tries to ensure.
Right.
 


> then will it iterate forwards?

No, it'd still iterate backwards, but starting from the wrong place - but
there is no correct place to start iterating from if there is no unused
element.
Thanks for the confirmation.

So I suppose we could have this in v1, attached.
With comments added by Tom.

regards,
Ranier Vilela
Attachment

pgsql-hackers by date:

Previous
From: Jacob Champion
Date:
Subject: Re: [PoC] Federated Authn/z with OAUTHBEARER
Next
From: Andres Freund
Date:
Subject: Re: Add more sanity checks around callers of changeDependencyFor()