On Thu, Sep 15, 2022 at 09:06:39AM +0000, Klemens Nanni wrote:
> On Thu, Sep 15, 2022 at 04:29:36AM -0400, Varik Valefor wrote:
> > This change removes a fairly large routine from a large function and places
> > the fairly large routine into a dedicated function. The decreasing of the
> > length of the main function should increase the readability of the large
> > function.
>
> This patch does not apply cleanly onto current CVS for me.
>
> >
> > Varik Valefor
> > "When lost, just pure :: Monad m => a -> m a."
> >
> > diff --git a/sbin/bioctl/bioctl.c b/sbin/bioctl/bioctl.c
> > index 58561e5f30f..19963ad762f 100644
> > --- a/sbin/bioctl/bioctl.c
> > +++ b/sbin/bioctl/bioctl.c
> > @@ -1307,6 +1307,29 @@ bcrypt_pbkdf_autorounds(void)
> >
> > return r;
> > }
> > +void
> > +readpassphrasefile(char *passphrase, FILE *f, size_t pl, struct stat sb)
> > +{
>
> *f, pl and sb are still declared in derive_key() but unused there,
> so they might as well be local to this new function.
>
> > + if ((f = fopen(password, "r")) == NULL)
> > + err(1, "invalid passphrase file");
> > +
> > + if (fstat(fileno(f), &sb) == -1)
> > + err(1, "can't stat passphrase file");
> > + if (sb.st_uid != 0)
> > + errx(1, "passphrase file must be owned by root");
> > + if ((sb.st_mode & ~S_IFMT) != (S_IRUSR | S_IWUSR))
> > + errx(1, "passphrase file has the wrong permissions");
> > +
> > + if (fgets(passphrase, sizeof(passphrase), f) == NULL)
Passphrase is a pointer here, so sizeof(passphrase) is 8 or 4 depending
on whether this is run on a 64-bit or a 32-bit arch. In other words,
this will only use the first 7 or 3 letters of the passphrase, which is
not ideal...
If we wanted to refactor this, the signature of the function should be:
void
readpassphrasefile(char *passphrase, size_t passphrase_len)
{
FILE *f;
struct stat sb;
size_t pl;
...
> > + err(1, "can't read passphrase file");
> > + pl = strlen(passphrase);
> > + if (pl > 0 && passphrase[pl - 1] == '\n')
> > + passphrase[pl - 1] = '\0';
> > + else
> > + errx(1, "invalid passphrase length");
> > +
> > + fclose(f);
> > +}
> >
> > void
> > derive_key(u_int32_t type, int rounds, u_int8_t *key, size_t keysz,
> > @@ -1331,25 +1354,7 @@ derive_key(u_int32_t type, int rounds, u_int8_t *key,
> > size_t keysz,
> >
> > /* get passphrase */
> > if (password) {
> > - if ((f = fopen(password, "r")) == NULL)
> > - err(1, "invalid passphrase file");
> > -
> > - if (fstat(fileno(f), &sb) == -1)
> > - err(1, "can't stat passphrase file");
> > - if (sb.st_uid != 0)
> > - errx(1, "passphrase file must be owned by root");
> > - if ((sb.st_mode & ~S_IFMT) != (S_IRUSR | S_IWUSR))
> > - errx(1, "passphrase file has the wrong
> > permissions");
> > -
> > - if (fgets(passphrase, sizeof(passphrase), f) == NULL)
here sizeof(passphrase) is 1024, since passphrase is an array of length
1024 of type char.
> > - err(1, "can't read passphrase file");
> > - pl = strlen(passphrase);
> > - if (pl > 0 && passphrase[pl - 1] == '\n')
> > - passphrase[pl - 1] = '\0';
> > - else
> > - errx(1, "invalid passphrase length");
> > -
> > - fclose(f);
> > + readpassphrasefile(passphrase, f, pl, sb);
Call this as
readpassphrasefile(passphrase, sizeof(passphrase));
> > } else {
> > if (readpassphrase(prompt, passphrase, sizeof(passphrase),
> > rpp_flag) == NULL)
> >
>
