Re: OpenSSL breaks factor(6)

> 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?

> 
> 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?

> -				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)

>  	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?

> +{
> +	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