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

[Zhang Boyang]

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.

> +                                                       \
> +  __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