All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] misc: Make grub_min() and grub_max() more resilient.
@ 2022-03-24 20:06 Peter Jones
  2022-03-24 22:43 ` Robbie Harwood
  2022-04-06 15:51 ` Daniel Kiper
  0 siblings, 2 replies; 3+ messages in thread
From: Peter Jones @ 2022-03-24 20:06 UTC (permalink / raw)
  To: grub-devel; +Cc: daniel.kiper

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);
             grub_disk_read (found.data->disk,
                             (grub_le_to_cpu32 (indirect_block_ptr[indirect_block]) * block_size) / GRUB_DISK_SECTOR_SIZE,
                             file->offset % block_size, read,
diff --git a/grub-core/loader/multiboot_elfxx.c b/grub-core/loader/multiboot_elfxx.c
index 6801f0ddf2b..8744be14a46 100644
--- a/grub-core/loader/multiboot_elfxx.c
+++ b/grub-core/loader/multiboot_elfxx.c
@@ -35,9 +35,7 @@
 #endif
 
 #include <grub/i386/relocator.h>
-
-#define CONCAT(a,b)	CONCAT_(a, b)
-#define CONCAT_(a,b)	a ## b
+#include <grub/misc.h>
 
 #pragma GCC diagnostic ignored "-Wcast-align"
 
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 7d2b5519690..83b31799722 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -35,6 +35,14 @@
 #define ARRAY_SIZE(array) (sizeof (array) / sizeof (array[0]))
 #define COMPILE_TIME_ASSERT(cond) switch (0) { case 1: case !(cond): ; }
 
+#ifndef CONCAT_
+#define CONCAT_(a, b) a ## b
+#endif
+
+#ifndef CONCAT
+#define CONCAT(a, b) CONCAT_(a, b)
+#endif
+
 #define grub_dprintf(condition, ...) grub_real_dprintf(GRUB_FILE, __LINE__, condition, __VA_ARGS__)
 
 void *EXPORT_FUNC(grub_memmove) (void *dest, const void *src, grub_size_t n);
@@ -495,8 +503,21 @@ void EXPORT_FUNC(grub_real_boot_time) (const char *file,
 #define grub_boot_time(...)
 #endif
 
-#define grub_max(a, b) (((a) > (b)) ? (a) : (b))
-#define grub_min(a, b) (((a) < (b)) ? (a) : (b))
+#define _grub_min(a, b, _a, _b)						      \
+  ({ typeof (a) _a = (a);						      \
+     typeof (b) _b = (b);						      \
+     _a < _b ? _a : _b; })
+#define grub_min(a, b) _grub_min(a, b,					      \
+				 CONCAT(_a_,__COUNTER__),		      \
+				 CONCAT(_b_,__COUNTER__))
+
+#define _grub_max(a, b, _a, _b)						      \
+  ({ typeof (a) _a = (a);						      \
+     typeof (b) _b = (b);						      \
+     _a > _b ? _a : _b; })
+#define grub_max(a, b) _grub_max(a, b,					      \
+				 CONCAT(_a_,__COUNTER__),		      \
+				 CONCAT(_b_,__COUNTER__))
 
 #define grub_log2ull(n) (GRUB_TYPE_BITS (grub_uint64_t) - __builtin_clzll (n) - 1)
 
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] misc: Make grub_min() and grub_max() more resilient.
  2022-03-24 20:06 [PATCH] misc: Make grub_min() and grub_max() more resilient Peter Jones
@ 2022-03-24 22:43 ` Robbie Harwood
  2022-04-06 15:51 ` Daniel Kiper
  1 sibling, 0 replies; 3+ messages in thread
From: Robbie Harwood @ 2022-03-24 22:43 UTC (permalink / raw)
  To: Peter Jones, grub-devel; +Cc: daniel.kiper

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

Peter Jones <pjones@redhat.com> writes:

> 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>

Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] misc: Make grub_min() and grub_max() more resilient.
  2022-03-24 20:06 [PATCH] misc: Make grub_min() and grub_max() more resilient Peter Jones
  2022-03-24 22:43 ` Robbie Harwood
@ 2022-04-06 15:51 ` Daniel Kiper
  1 sibling, 0 replies; 3+ messages in thread
From: Daniel Kiper @ 2022-04-06 15:51 UTC (permalink / raw)
  To: Peter Jones; +Cc: grub-devel, rharwood

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


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-06 15:52 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-24 20:06 [PATCH] misc: Make grub_min() and grub_max() more resilient Peter Jones
2022-03-24 22:43 ` Robbie Harwood
2022-04-06 15:51 ` Daniel Kiper

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.