Re: [hackers] [sbase] [PATCH 01/10] crypt: Add some missing error checks for cryptsum

From: Silvan Jegen <s.jegen_AT_gmail.com>
Date: Mon, 5 Dec 2016 11:02:38 +0100

Hi

And thanks for the patches!

Comments below.

On Mon, Dec 5, 2016 at 6:55 AM, Michael Forney <mforney_AT_mforney.org> wrote:
> Previously, if a file failed to read in a checksum list, it would be
> reported as not matched rather than a read failure.
>
> Also, if reading from stdin failed, previously a bogus checksum would be
> printed anyway.
> ---
> libutil/crypt.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/libutil/crypt.c b/libutil/crypt.c
> index 3f849ba..6991c39 100644
> --- a/libutil/crypt.c
> +++ b/libutil/crypt.c
> _AT_@ -64,7 +64,10 @@ mdchecklist(FILE *listfp, struct crypt_ops *ops, uint8_t *md, size_t sz,
> (*noread)++;
> continue;
> }
> - cryptsum(ops, fp, file, md);
> + if (cryptsum(ops, fp, file, md)) {
> + (*noread)++;
> + continue;
> + }
> r = mdcheckline(line, md, sz);
> if (r == 1) {
> printf("%s: OK\n", file);
> _AT_@ -125,8 +128,10 @@ cryptmain(int argc, char *argv[], struct crypt_ops *ops, uint8_t *md, size_t sz)
> int ret = 0;
>
> if (argc == 0) {
> - cryptsum(ops, stdin, "<stdin>", md);
> - mdprint(md, "<stdin>", sz);
> + if (cryptsum(ops, stdin, "<stdin>", md))
> + ret = 1;

The return values in crypt.c are not consistent which tripped me up here.

"cryptsum" returns 1 on error (as does "cryptmain") but "mdcheckline"
returns 0 (or -1) on error and 1 on success. For consistency we should
change "mdcheckline" to return 1/-1 on error and 0 on success.

If we make that change it should be in a different patch though.

> + else
> + mdprint(md, "<stdin>", sz);
> } else {
> for (; *argv; argc--, argv++) {
> if ((*argv)[0] == '-' && !(*argv)[1]) {
> _AT_@ -137,11 +142,10 @@ cryptmain(int argc, char *argv[], struct crypt_ops *ops, uint8_t *md, size_t sz)
> ret = 1;
> continue;
> }
> - if (cryptsum(ops, fp, *argv, md)) {
> + if (cryptsum(ops, fp, *argv, md))
> ret = 1;
> - } else {
> + else
> mdprint(md, *argv, sz);
> - }

This is a style change and some people may thus want to have it put in
a separate patch. I think combining such a minor change with this
patch is fine.

This patch LGTM!


Cheers,

Silvan


> if (fp != stdin && fshut(fp, *argv))
> ret = 1;
> }
> --
> 2.11.0
>
>
Received on Mon Dec 05 2016 - 11:02:38 CET