Re: [PATCH] grub-mkpasswd-pbkdf2: Simplify the main function implementation

[Glenn Washburn]

On Sun, 14 Mar 2021 19:09:36 +0800
Tianjia Zhang <tianjia.zhang@linux.alibaba.com> wrote:

> ping.
> 
> Thanks,
> Tianjia
> 
> On 11/18/20 2:58 PM, Tianjia Zhang wrote:
> > Allocate memory if needed, while saving the corresponding release
> > operation, reducing the amount of code and code complexity.
> > 
> > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
> > ---
> >   util/grub-mkpasswd-pbkdf2.c | 20 +++++---------------
> >   1 file changed, 5 insertions(+), 15 deletions(-)
> > 
> > diff --git a/util/grub-mkpasswd-pbkdf2.c
> > b/util/grub-mkpasswd-pbkdf2.c index 5805f3c10..cd7943df6 100644
> > --- a/util/grub-mkpasswd-pbkdf2.c
> > +++ b/util/grub-mkpasswd-pbkdf2.c
> > @@ -132,35 +132,25 @@ main (int argc, char *argv[])
> >         fprintf (stderr, "%s", _("Error in parsing command line
> > arguments\n")); exit(1);
> >       }
> > -
> > -  buf = xmalloc (arguments.buflen);
> > -  salt = xmalloc (arguments.saltlen);
> >     
> >     printf ("%s", _("Enter password: "));
> >     if (!grub_password_get (pass1, GRUB_AUTH_MAX_PASSLEN))
> > -    {
> > -      free (buf);
> > -      free (salt);
> > -      grub_util_error ("%s", _("failure to read password"));
> > -    }
> > +    grub_util_error ("%s", _("failure to read password"));
> >     printf ("%s", _("Reenter password: "));
> >     if (!grub_password_get (pass2, GRUB_AUTH_MAX_PASSLEN))
> > -    {
> > -      free (buf);
> > -      free (salt);
> > -      grub_util_error ("%s", _("failure to read password"));
> > -    }
> > +    grub_util_error ("%s", _("failure to read password"));
> >   
> >     if (strcmp (pass1, pass2) != 0)
> >       {
> >         memset (pass1, 0, sizeof (pass1));
> >         memset (pass2, 0, sizeof (pass2));
> > -      free (buf);
> > -      free (salt);
> >         grub_util_error ("%s", _("passwords don't match"));
> >       }
> >     memset (pass2, 0, sizeof (pass2));
> >   
> > +  buf = xmalloc (arguments.buflen);
> > +  salt = xmalloc (arguments.saltlen);
> > +
> >     if (grub_get_random (salt, arguments.saltlen))
> >       {
> >         memset (pass1, 0, sizeof (pass1));
> > 

LGTM. I wonder if there's some conventions as with local variable
declaration, where we try to have memory allocation near the beginning
of the function as a kind of "declaration" that this should be freed
before leaving the function.

  Reviewed-by: Glenn Washburn <development@efficientek.com>

Glenn