Re: [PATCH v2] normal/menu_entry: Add grub_calloc failure check

Re: [PATCH v2] normal/menu_entry: Add grub_calloc failure check

[Avnish Chouhan]

On 2025-12-02 12:32, grub-devel-request@gnu.org wrote:
> Message: 3
> Date: Tue,  2 Dec 2025 12:31:48 +0530
> From: Sridhar Markonda <sridharm@linux.ibm.com>
> To: grub-devel@gnu.org
> Cc: daniel.kiper@oracle.com, phcoder@gmail.com,
> 	sudhakar@linux.ibm.com, stefanb@linux.ibm.com, nayna@linux.ibm.com,
> 	sridharm@linux.ibm.com, ssrish@linux.ibm.com
> Subject: [PATCH v2] normal/menu_entry: Add grub_calloc failure check
> Message-ID: <20251202070148.35202-1-sridharm@linux.ibm.com>
> 
> Add a NULL check to prevent failure from grub_calloc in update_screen.
> This stops possible NULL pointer dereference and avoids unexpected 
> behavior.
> 
> Signed-off-by: Sridhar Markonda <sridharm@linux.ibm.com>
> ---
>  grub-core/normal/menu_entry.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/normal/menu_entry.c 
> b/grub-core/normal/menu_entry.c
> index 8b0d17e3f..411b0ad53 100644
> --- a/grub-core/normal/menu_entry.c
> +++ b/grub-core/normal/menu_entry.c
> @@ -295,8 +295,12 @@ update_screen (struct screen *screen, struct
> per_term_screen *term_screen,
> 
>  	  pos = linep->pos + (term_screen - screen->terms);
> 
> -	  if (!*pos)
> -	    *pos = grub_calloc (linep->len + 1, sizeof (**pos));
> +	  if (*pos == NULL)
> +	    {
> +	      *pos = grub_calloc (linep->len + 1, sizeof (**pos));
> +	      if (*pos == NULL)
> +		return;

Hi Sridhar,
Thank you so much for the patch!

In my opinion, returning due to grub_calloc failure might not be a good 
idea here!

To me, it seems grub_print_ucs4_menu() and its corresponding functions 
which uses 'pos' as a last argument handles NULL properly. So returning 
based on this argument being NULL might not be a good idea. There are 
other parts of the code in GRUB where grub_print_ucs4_menu() has been 
called with this last argument as 0.

Regards,
Avnish Chouhan


> +	    }
> 
>  	  if (i == region_start || linep == screen->lines + screen->line
>  	      || (i > region_start && mode == ALL_LINES))
> --
> 2.47.3
> 
> 
> 
> 
> ------------------------------
> 
> Subject: Digest Footer
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> 
> ------------------------------
> 
> End of Grub-devel Digest, Vol 262, Issue 5
> ******************************************

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2] normal/menu_entry: Add grub_calloc failure check

[Daniel Kiper]

On Tue, Dec 02, 2025 at 11:03:12PM +0530, Avnish Chouhan wrote:
> On 2025-12-02 12:32, grub-devel-request@gnu.org wrote:
> > Message: 3
> > Date: Tue,  2 Dec 2025 12:31:48 +0530
> > From: Sridhar Markonda <sridharm@linux.ibm.com>
> > To: grub-devel@gnu.org
> > Cc: daniel.kiper@oracle.com, phcoder@gmail.com,
> > 	sudhakar@linux.ibm.com, stefanb@linux.ibm.com, nayna@linux.ibm.com,
> > 	sridharm@linux.ibm.com, ssrish@linux.ibm.com
> > Subject: [PATCH v2] normal/menu_entry: Add grub_calloc failure check
> > Message-ID: <20251202070148.35202-1-sridharm@linux.ibm.com>
> >
> > Add a NULL check to prevent failure from grub_calloc in update_screen.
> > This stops possible NULL pointer dereference and avoids unexpected
> > behavior.
> >
> > Signed-off-by: Sridhar Markonda <sridharm@linux.ibm.com>
> > ---
> >  grub-core/normal/menu_entry.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/grub-core/normal/menu_entry.c
> > b/grub-core/normal/menu_entry.c
> > index 8b0d17e3f..411b0ad53 100644
> > --- a/grub-core/normal/menu_entry.c
> > +++ b/grub-core/normal/menu_entry.c
> > @@ -295,8 +295,12 @@ update_screen (struct screen *screen, struct
> > per_term_screen *term_screen,
> >
> >  	  pos = linep->pos + (term_screen - screen->terms);
> >
> > -	  if (!*pos)
> > -	    *pos = grub_calloc (linep->len + 1, sizeof (**pos));
> > +	  if (*pos == NULL)
> > +	    {
> > +	      *pos = grub_calloc (linep->len + 1, sizeof (**pos));
> > +	      if (*pos == NULL)
> > +		return;
>
> Hi Sridhar,
> Thank you so much for the patch!
>
> In my opinion, returning due to grub_calloc failure might not be a good idea
> here!
>
> To me, it seems grub_print_ucs4_menu() and its corresponding functions which
> uses 'pos' as a last argument handles NULL properly. So returning based on
> this argument being NULL might not be a good idea. There are other parts of
> the code in GRUB where grub_print_ucs4_menu() has been called with this last
> argument as 0.

Yeah, I think Avnish is right.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2] normal/menu_entry: Add grub_calloc failure check

[Sridhar Markonda]

Add a NULL check to prevent failure from grub_calloc in update_screen.
This stops possible NULL pointer dereference and avoids unexpected behavior.

Signed-off-by: Sridhar Markonda <sridharm@linux.ibm.com>
---
 grub-core/normal/menu_entry.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/grub-core/normal/menu_entry.c b/grub-core/normal/menu_entry.c
index 8b0d17e3f..411b0ad53 100644
--- a/grub-core/normal/menu_entry.c
+++ b/grub-core/normal/menu_entry.c
@@ -295,8 +295,12 @@ update_screen (struct screen *screen, struct per_term_screen *term_screen,
 
 	  pos = linep->pos + (term_screen - screen->terms);
 
-	  if (!*pos)
-	    *pos = grub_calloc (linep->len + 1, sizeof (**pos));
+	  if (*pos == NULL)
+	    {
+	      *pos = grub_calloc (linep->len + 1, sizeof (**pos));
+	      if (*pos == NULL)
+		return;
+	    }
 
 	  if (i == region_start || linep == screen->lines + screen->line
 	      || (i > region_start && mode == ALL_LINES))
-- 
2.47.3


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel