Re: [PATCH v14 1/3] safemath: Add ALIGN_UP_OVF() that checks for {over, under}flow

[Zhang Boyang]

Hi,

On 2024/5/18 15:52, Zhang Boyang wrote:
> Hi,
>
> On 2024/5/17 20:56, Gao Xiang wrote:
> > The following EROFS patch will use this helper to handle overflow
> > ALIGN_UP() cases.
> >
> > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com>
> > ---
> >   include/grub/safemath.h | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/include/grub/safemath.h b/include/grub/safemath.h
> > index fbd9b5925..baaea0ef4 100644
> > --- a/include/grub/safemath.h
> > +++ b/include/grub/safemath.h
> > @@ -32,6 +32,22 @@
> >   #define grub_cast(a, res)    grub_add ((a), 0, (res))
> > +#define ALIGN_UP_OVF(v, align, res)             \
> > +({                            \
> > +  bool __failed;                    \
> > +  typeof(v) a;                        \
>
> I think "a" should be changed to some unique name like "__a", because 
> variable shadowing might occur:
>
> int a = 10, b = 4;
> if (!ALIGN_UP_OVF(a, b, &a)) { // "a" is shadowed
>    printf("a=%d\n", a);
> }
>
> The uniqueness of variable name can reduce the possibility of variable 
> shadowing, but it will not eliminate the pitfall.
> https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
>
> The ultimate solution would be a macro-like "static inline" function, 
> but I'm not sure whether a inline function is appropriate in grub headers.

Oh, sorry, I'm wrong. This marco can't be a inline function because 
function signature requires specific types to be given to a variable, 
which will eliminate the efforts of overflow macros.

By the way, I think it would be better to write a comment about "align 
should not equal to 0 and it must be a power of 2"

Zhang Boyang

> > +                            \
> > +  __failed = grub_sub ((typeof(v))(align), 1, &a);    \
>
> The cast "(typeof(v))" is unnecessary and it will silently ignore 
> overflow/underflow.
>
> Zhang Boyang
>
> > +  if (__failed == false)                \
> > +    {                            \
> > +    __failed = grub_add (v, a, res);            \
> > +    if (__failed == false)                \
> > +      {                            \
> > +        *(res) &= ~a;                    \
> > +      }                            \
> > +    }                            \
> > +__failed;})
> > +
> >   #else
> >   #error gcc 5.1 or newer or clang 8.0 or newer is required
> >   #endif