[PATCH] misc: Set grub_errno on all failures in grub_xvasprintf()

[PATCH] misc: Set grub_errno on all failures in grub_xvasprintf()

[Glenn Washburn]

When failing to allocate the preallocate buffer, grub_xvasprintf()
returns NULL, but does not set grub_errno. Returning NULL is sufficient
for a caller to determine there was an error. However, some usages of
grub_xvasprintf() check for a NULL return value and then return
grub_errno, which could return some previously set error code or
potentially even a success code.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/kern/misc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index b57249acb81b..afb41bd63a6a 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -1232,6 +1232,7 @@ grub_xvasprintf (const char *fmt, va_list ap)
       if (!ret)
 	{
 	  free_printf_args (&args);
+	  grub_error (GRUB_ERR_OUT_OF_MEMORY, "grub_xvasprintf failed to allocate memory of size %" PRIuGRUB_SIZE, as);
 	  return NULL;
 	}
 
-- 
2.34.1


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

Re: [PATCH] misc: Set grub_errno on all failures in grub_xvasprintf()

[Daniel Kiper]

On Fri, Sep 01, 2023 at 09:16:24PM -0500, Glenn Washburn wrote:
> When failing to allocate the preallocate buffer, grub_xvasprintf()
> returns NULL, but does not set grub_errno. Returning NULL is sufficient
> for a caller to determine there was an error. However, some usages of
> grub_xvasprintf() check for a NULL return value and then return
> grub_errno, which could return some previously set error code or
> potentially even a success code.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

> ---
>  grub-core/kern/misc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> index b57249acb81b..afb41bd63a6a 100644
> --- a/grub-core/kern/misc.c
> +++ b/grub-core/kern/misc.c
> @@ -1232,6 +1232,7 @@ grub_xvasprintf (const char *fmt, va_list ap)
>        if (!ret)
>  	{
>  	  free_printf_args (&args);
> +	  grub_error (GRUB_ERR_OUT_OF_MEMORY, "grub_xvasprintf failed to allocate memory of size %" PRIuGRUB_SIZE, as);

Nit, s/grub_xvasprintf/grub_xvasprintf()/...

Daniel

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

Re: [PATCH] misc: Set grub_errno on all failures in grub_xvasprintf()

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 1474 bytes --]

2 problems:
1) Did you audit that all call places either clear or propagate error?
2) We use standard error message "out of memory". This allows it to be
translated and not be too technical.

Le sam. 2 sept. 2023, 04:17, Glenn Washburn <development@efficientek.com> a
écrit :

> When failing to allocate the preallocate buffer, grub_xvasprintf()
> returns NULL, but does not set grub_errno. Returning NULL is sufficient
> for a caller to determine there was an error. However, some usages of
> grub_xvasprintf() check for a NULL return value and then return
> grub_errno, which could return some previously set error code or
> potentially even a success code.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/kern/misc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> index b57249acb81b..afb41bd63a6a 100644
> --- a/grub-core/kern/misc.c
> +++ b/grub-core/kern/misc.c
> @@ -1232,6 +1232,7 @@ grub_xvasprintf (const char *fmt, va_list ap)
>        if (!ret)
>         {
>           free_printf_args (&args);
> +         grub_error (GRUB_ERR_OUT_OF_MEMORY, "grub_xvasprintf failed to
> allocate memory of size %" PRIuGRUB_SIZE, as);
>           return NULL;
>         }
>
> --
> 2.34.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2190 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

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

Re: [PATCH] misc: Set grub_errno on all failures in grub_xvasprintf()

[Glenn Washburn]

On Sat, 2 Sep 2023 22:03:08 +0200
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:

> 2 problems:
> 1) Did you audit that all call places either clear or propagate error?

Am I correct in understanding that you're asking if all calls to
grub_xvasprintf() with clear grub_errno or return it? I have not
checked that. There's not many calls of grub_xvasprintf(), but there's
138 calls to grub_xasprintf(). I have checked a few and it looks like
many return grub_errno when grub_xasprintf() returns NULL right now.
So those would currently be returning either SUCCESS when they
shouldn't or a failure code that is not what it should be. Those
already assume that grub_xvasprintf() works as if this patch were
applied.

Is your concern that there is code that relies on this
bug in grub_xvasprintf()? For instance, that some function returns
grub_errno after a failed grub_xvasprintf(), but the callee expects to
receive some prior error code instead of the out of memory error?

> 2) We use standard error message "out of memory". This allows it to be
> translated and not be too technical.

I made it more specific so its easier to tell where the out of memory
is coming from. I suppose this could be done better in other ways, for
instance adding file and line number to all grub_error calls like is
done in redhat's grub repo. So I'll change this.

Glenn

> Le sam. 2 sept. 2023, 04:17, Glenn Washburn <development@efficientek.com> a
> écrit :
> 
> > When failing to allocate the preallocate buffer, grub_xvasprintf()
> > returns NULL, but does not set grub_errno. Returning NULL is sufficient
> > for a caller to determine there was an error. However, some usages of
> > grub_xvasprintf() check for a NULL return value and then return
> > grub_errno, which could return some previously set error code or
> > potentially even a success code.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/kern/misc.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> > index b57249acb81b..afb41bd63a6a 100644
> > --- a/grub-core/kern/misc.c
> > +++ b/grub-core/kern/misc.c
> > @@ -1232,6 +1232,7 @@ grub_xvasprintf (const char *fmt, va_list ap)
> >        if (!ret)
> >         {
> >           free_printf_args (&args);
> > +         grub_error (GRUB_ERR_OUT_OF_MEMORY, "grub_xvasprintf failed to
> > allocate memory of size %" PRIuGRUB_SIZE, as);
> >           return NULL;
> >         }
> >
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >

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