Re: [PATCH] misc: Make grub_min() and grub_max() more resilient.

[Daniel Kiper]

On Thu, Mar 24, 2022 at 04:06:38PM -0400, Peter Jones wrote:
> grub_min(a,b) and grub_max(a,b) use a relatively naive implementation
> which leads to several problems:
> - they evaluate their parameters more than once
> - the naive way to address this, to declare temporary variables in a
>   statement-expression, isn't resilient against nested uses, because
>   MIN(a,MIN(b,c)) results in the temporary variables being declared in
>   two nested scopes, which may result in a build warning depending on
>   your build options.
>
> This patch changes our implementation to use a statement-expression
> inside a helper macro, and creates the symbols for the temporary
> variables with __COUNTER__ (A GNU C cpp extension) and token pasting to
> create uniquely named internal variables.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
>  grub-core/fs/reiserfs.c            | 28 +++++++++-------------------
>  grub-core/loader/multiboot_elfxx.c |  4 +---
>  include/grub/misc.h                | 25 +++++++++++++++++++++++--
>  3 files changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/grub-core/fs/reiserfs.c b/grub-core/fs/reiserfs.c
> index 36b26ac98a0..42818c37622 100644
> --- a/grub-core/fs/reiserfs.c
> +++ b/grub-core/fs/reiserfs.c
> @@ -42,16 +42,6 @@
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> -#define MIN(a, b) \
> -  ({ typeof (a) _a = (a); \
> -     typeof (b) _b = (b); \
> -     _a < _b ? _a : _b; })
> -
> -#define MAX(a, b) \
> -  ({ typeof (a) _a = (a); \
> -     typeof (b) _b = (b); \
> -     _a > _b ? _a : _b; })
> -
>  #define REISERFS_SUPER_BLOCK_OFFSET 0x10000
>  #define REISERFS_MAGIC_LEN 12
>  #define REISERFS_MAGIC_STRING "ReIsEr"
> @@ -1076,7 +1066,7 @@ grub_reiserfs_read_real (struct grub_fshelp_node *node,
>    grub_reiserfs_set_key_type (&key, GRUB_REISERFS_ANY, 2);
>    initial_position = off;
>    current_position = 0;
> -  final_position = MIN (len + initial_position, node->size);
> +  final_position = grub_min (len + initial_position, node->size);
>    grub_dprintf ("reiserfs",
>               "Reading from %lld to %lld (%lld instead of requested %ld)\n",
>               (unsigned long long) initial_position,
> @@ -1115,8 +1105,8 @@ grub_reiserfs_read_real (struct grub_fshelp_node *node,
>            grub_dprintf ("reiserfs_blocktype", "D: %u\n", (unsigned) block);
>            if (initial_position < current_position + item_size)
>              {
> -              offset = MAX ((signed) (initial_position - current_position), 
> 0);
> -              length = (MIN (item_size, final_position - current_position)
> +              offset = grub_max ((signed) (initial_position - 
> current_position), 0);
> +              length = (grub_min (item_size, final_position - 
> current_position)
>                          - offset);
>                grub_dprintf ("reiserfs",
>                              "Reading direct block %u from %u to %u...\n",
> @@ -1161,9 +1151,9 @@ grub_reiserfs_read_real (struct grub_fshelp_node *node,
>                grub_dprintf ("reiserfs_blocktype", "I: %u\n", (unsigned) 
> block);
>                if (current_position + block_size >= initial_position)
>                  {
> -                  offset = MAX ((signed) (initial_position - 
> current_position),
> -                                0);
> -                  length = (MIN (block_size, final_position - 
> current_position)
> +                  offset = grub_max ((signed) (initial_position - 
> current_position),
> +                                  0);
> +                  length = (grub_min (block_size, final_position - 
> current_position)
>                              - offset);
>                    grub_dprintf ("reiserfs",
>                                  "Reading indirect block %u from %u to 
> %u...\n",
> @@ -1205,7 +1195,7 @@ grub_reiserfs_read_real (struct grub_fshelp_node *node,
>    switch (found.type)
>      {
>        case GRUB_REISERFS_DIRECT:
> -        read_length = MIN (len, item_size - file->offset);
> +        read_length = grub_min (len, item_size - file->offset);
>          grub_disk_read (found.data->disk,
>                          (found.block_number * block_size) / 
> GRUB_DISK_SECTOR_SIZE,
>                          grub_le_to_cpu16 (found.header.item_location) + 
> file->offset,
> @@ -1224,12 +1214,12 @@ grub_reiserfs_read_real (struct grub_fshelp_node 
> *node,
>                          item_size, (char *) indirect_block_ptr);
>          if (grub_errno)
>            goto fail;
> -        len = MIN (len, file->size - file->offset);
> +        len = grub_min (len, file->size - file->offset);
>          for (indirect_block = file->offset / block_size;
>               indirect_block < indirect_block_count && read_length < len;
>               indirect_block++)
>            {
> -            read = MIN (block_size, len - read_length);
> +            read = grub_min (block_size, len - read_length);

I am OK with these changes but they should be done in separate patch or
mentioned in the commit message you are doing them on the occasion.

If you do that feel free to add my RB to the patch(es).

Daniel