Hi, On Mon, Apr 26, 2021 at 09:58:38PM +0200, Antonio Quartulli wrote: > > Rewrite error message to differenciate "hash too short" (including > differenciate -> differenTiate
fixed.
> > - int i;
> > + int i=0;
>
> spaces around the '='
fixed.
> > - if (strlen(cp) < 2)
> > + /* valid segments consist of exactly two hex digits, then ':' or
> > EOS */
> > + if (!isxdigit(cp[0])
> > + || !isxdigit(cp[1])
>
> since we have a limit of 80 chars per line, how about putting the two
> conditions above on the same line?
I tried, but found it "not easier to read".
The way it is, it's really "3 lines for the 3 checks for the 3 bytes"
(and then one check for "all of it"), so I find it more logical.
> Regarding the format string, we concluded on IRC that %hhx is part of
> C99 - why not using it here then?
As I said there: C99 is a compiler standard, but %hhx is something the
library has to provide. I do not trust C libraries in that regard -
imagine someone compiling OpenVPN with a nice and shiny C99 compiler
and then linking with some stripped down C library for an embedded
system, which doesn't do %hhx. Interesting memory corruption, impossible
to diagnose.
Now, I have no idea whether uc-libc, musl, ... *do* support %hhx, but
I do not see use of this as particular readability-enhancing while it
carries a portability risk. %x -> (int) works, everywhere.
> > + ret->hash[i++] = (uint8_t)byte;
>
> This cast would go away if byte is declared as uint8_t and we use %hhx
> in the sscanf().
We could sscanf() to ret->hash[i] right away, then - but again, I
do not think the extra step would be better for readability, and it
does carry a risk.
> > - if (term == 0)
> > + term = cp[2];
> > + if (term == '\0')
>
> do we really need "term" at al at this point?
> we could directly use cp[2] in the if condition.
If we exit the loop because we have reached (i == nbytes), cp will
point at the character "after the end-of-string" (because of cp +=3),
so cp[2] is well into "undefined territory" land.
> > + if (i < nbytes)
> > {
> > - msg(msglevel, "hash fingerprint is different length than expected
> > (%d bytes): %s", nbytes, str);
> > + msg(msglevel, "hash fingerprint is wrong length - expected %d
> > bytes, got %d: %s", nbytes, i, str);
> > + }
> > + else if (term != '\0')
>
> same here: if we hit the else branch, it means that cp[0,1,2] are all
> defined so we can again compare cp[2] to '\0'.
Actually, no. We hit this branch when we exit the loop due to
(i==nbytes) after the cp +=3; This happens if you have malformed
input that is too long, so after having found "nbytes", term is ":",
so cp += 3, and then the loop ends.
Pure OpenVPN style would be, of course, to do the cp += 3 first
(and alway), and then look at cp[-1], but no, I'm not going there.
> Should we also print options->verify_hash_depth while at it?
Done!
v2 incoming... including a bugfix to the original commit :)
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
