Date: Fri, 27 Dec 2019 19:51:55 -0800 From: Steve Kargl <sgk@troutmask.apl.washington.edu> To: "Rodney W. Grimes" <freebsd-rwg@gndrsh.dnsmgr.net> Cc: freebsd-current@freebsd.org Subject: Re: OpenSSL breaks factor(6) Message-ID: <20191228035155.GA62416@troutmask.apl.washington.edu> In-Reply-To: <201912280300.xBS304bB041043@gndrsh.dnsmgr.net> References: <20191227224212.GA61594@troutmask.apl.washington.edu> <201912280300.xBS304bB041043@gndrsh.dnsmgr.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Dec 27, 2019 at 07:00:04PM -0800, Rodney W. Grimes wrote:
> > On Fri, Dec 27, 2019 at 01:47:17PM -0800, Steve Kargl wrote:
> > > On Fri, Dec 27, 2019 at 01:25:30PM -0800, Steve Kargl wrote:
> > > > The use of OpenSSL in factor(6) breaks factor(6) with respect to
> > > > its documentation.
> > > >
> > > > % man factor
> > > > ...
> > > > Numbers may be preceded by a single '+'.
> > > > ...
> > > >
> > > > % factor +125
> > > > factor: +125: illegal numeric format.
> > > >
> > >
> > > This fixes factor(6) for the above issue. The issue with
> > > hexadecimal is not easily fixed.
> > >
> >
> > This patch now includes a fix for hexadecimal conversion. It
> > simple scans the string for a hex digit in [a,...,f] and assumes
> > that a hexadecimal string has been entered. A string that includes
> > character from the decimal digits is assumed to by a decimal
> > representation.
>
> It looks to me that the old code did the common method of
> try to convert as decimal, if that fails, try it as hex,
> if that fails report an error.
>
> Why is is that this common logic no longer works?
AFAICT, BN_dec2bn and BN_hex2bn from OpenSSL scan from left
to right, does a conversion with what is possible, and reports
success. That is, for 1abc, BN_dec2bn can convert 1 to 1 and
reports success. The local implementations of these functions,
when OpenSSL is not used, does not do this partial conversion.
> >
> > Index: factor.c
> > ===================================================================
> > --- factor.c (revision 355983)
> > +++ factor.c (working copy)
> > @@ -71,6 +71,7 @@
> > #include <errno.h>
> > #include <inttypes.h>
> > #include <limits.h>
> > +#include <stdbool.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <unistd.h>
> > @@ -104,6 +105,7 @@
> >
> > #endif
> >
> > +static bool is_hex(char *str);
> > static void BN_print_dec_fp(FILE *, const BIGNUM *);
> >
> > static void pr_fact(BIGNUM *); /* print factors of a value */
> > @@ -148,21 +150,25 @@
> > for (p = buf; isblank(*p); ++p);
> > if (*p == '\n' || *p == '\0')
> > continue;
> > + if (*p == '+') p++;
> > if (*p == '-')
> > errx(1, "negative numbers aren't permitted.");
> > - if (BN_dec2bn(&val, buf) == 0 &&
> > - BN_hex2bn(&val, buf) == 0)
>
> Why does this logic fail?
See BN_hex2bn manpage. C/C++ does shortcircuits. With 1abc,
BN_dec2bn converts the string to 1, puts it in val, and returns
nonzero. BN_hex2bn is never called. Flipping the conditionals,
of course, doesn't work because 0-9 are digits in the hexadecimal
set (e.g., 111 is a valid hex and decimal string).
> > - errx(1, "%s: illegal numeric format.", buf);
> > + ch = is_hex(p) ? BN_hex2bn(&val, p) :
> > + BN_dec2bn(&val, p);
> > + if (ch == 0)
> > + errx(1, "%s: illegal numeric format.", p);
> > pr_fact(val);
> > }
> > /* Factor the arguments. */
> > else
> > - for (; *argv != NULL; ++argv) {
> > - if (argv[0][0] == '-')
> > + for (p = *argv; p != NULL; p = *++argv) {
> > + if (*p == '-')
> > errx(1, "negative numbers aren't permitted.");
> > - if (BN_dec2bn(&val, argv[0]) == 0 &&
> > - BN_hex2bn(&val, argv[0]) == 0)
> > - errx(1, "%s: illegal numeric format.", argv[0]);
> > + if (*p == '+') p++;
> > + ch = is_hex(p) ? BN_hex2bn(&val, p) :
> > + BN_dec2bn(&val, p);
> > + if (ch == 0)
> > + errx(1, "%s: illegal numeric format.", p);
> > pr_fact(val);
> > }
> > exit(0);
> > @@ -343,10 +349,9 @@
> > BN_dec2bn(BIGNUM **a, const char *str)
> > {
> > char *p;
> > -
> This blank line is part of style(9)
>
Whoops. Haven't had to worry about style(9) in a long time.
> > errno = 0;
> > **a = strtoul(str, &p, 10);
> > - return (errno == 0 && (*p == '\n' || *p == '\0'));
> > + return (errno == 0 ? 1 : 0); /* OpenSSL returns 0 on error! */
> > }
> >
> > static int
> > @@ -356,7 +361,7 @@
> >
> > errno = 0;
> > **a = strtoul(str, &p, 16);
> > - return (errno == 0 && (*p == '\n' || *p == '\0'));
> > + return (errno == 0 ? 1 : 0); /* OpenSSL returns 0 on error! */
> > }
> >
> > static BN_ULONG
> > @@ -370,3 +375,17 @@
> > }
> >
> > #endif
> > +
> > +/* Check if the string contains a hexadecimal digit. */
> > +static bool
> > +is_hex(char *str)
> This function is poorly named as it does not check for
> all valid hex digits, only for alpha hex digits. It
> also only accepts lower case hex alpha, I would expect
> hex input to be case insensitive.
>
> is_hexalpha?
Feel free to rename it and improve.
>
> > +{
> > + char c, *p;
> > + for (p = str; *p; p++) {
> > + c = tolower(*p);
> > + if (c == 'a' || c == 'b' || c == 'c' || c == 'd' ||
> > + c == 'e' || c == 'f')
> if ( c >= 'a' || c <= 'f')
>
> > + return true;
> > + }
> > + return false;
> > +}
> >
> > --
> > Steve
>
> --
> Rod Grimes rgrimes@freebsd.org
--
Steve
20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
20161221 https://www.youtube.com/watch?v=IbCHE-hONow
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191228035155.GA62416>