Re: OpenSSL breaks factor(6)

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)
> > _at__at_ -71,6 +71,7 _at__at_
> >  #include <errno.h>
> >  #include <inttypes.h>
> >  #include <limits.h>
> > +#include <stdbool.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> >  #include <unistd.h>
> > _at__at_ -104,6 +105,7 _at__at_
> >  
> >  #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 */
> > _at__at_ -148,21 +150,25 _at__at_
> >  			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);
> > _at__at_ -343,10 +349,9 _at__at_
> >  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
> > _at__at_ -356,7 +361,7 _at__at_
> >  
> >  	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
> > _at__at_ -370,3 +375,17 _at__at_
> >  }
> >  
> >  #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_at_freebsd.org

-- 
Steve
20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
20161221 https://www.youtube.com/watch?v=IbCHE-hONow