[ccan] [PATCH] opt/helpers: fix out-of-range check in opt_set_floatval()

[Douglas Bagnall]

opt_set_floatval() uses opt_set_doubleval() to do initial conversion
and bounds checking before casting the result to float.  Previously
the out of bounds check compared the original and float values for
equality and declared anything unequal to be out of bounds. While this
trick works well in the orderly integer world, it can backfire with
floating point. For example, 3.1 resolves to the double more precisely
known as 3.100000000000000088817841970012523233890533447265625, while
3.1f resolves to 3.099999904632568359375.  These are not equal, though
3.1 is generally regarded as being in bounds for a float.  There are
around 8 billion other doubles (i.e. 3.1 +/- a little bit) that map to
the same 3.1f value, of which only one is strictly equal to it.

Why wasn't this discovered by the tests? It turns out that neither
set_floatval nor set_doubleval were tested against non-integral
numbers.  This is slightly improved here.

This patch uses the arguably more reasonable definition of bounds that
is found in opt_set_doubleval(): it excludes numbers that would get
rounded to zero or an infinity. One subtlety is that the double
version allows `--foo=INF` for an explicit infinity without overflow.
This is possibly useful, and there is some fiddling to allow this for
floats. Likewise an explicit zero is allowed, as you would expect.

It is perhaps worth noting that the `*f = d` cast/assignment at the
heart of it all can crash and core dump on overflow or underflow if
the floating point exception flags are in an unexpected state.

Signed-off-by: Douglas Bagnall <douglas at halo.gen.nz>
---
 ccan/opt/helpers.c          | 11 +++++++++--
 ccan/opt/test/run-helpers.c | 29 ++++++++++++++++++++++++++++-
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/ccan/opt/helpers.c b/ccan/opt/helpers.c
index 43b86d7..f247301 100644
--- a/ccan/opt/helpers.c
+++ b/ccan/opt/helpers.c
@@ -9,6 +9,7 @@
 #include <stdio.h>
 #include <limits.h>
 #include "private.h"
+#include <float.h>
 
 /* Upper bound to sprintf this simple type?  Each 3 bits < 1 digit. */
 #define CHAR_SIZE(type) (((sizeof(type)*CHAR_BIT + 2) / 3) + 1)
@@ -126,8 +127,14 @@ char *opt_set_floatval(const char *arg, float *f)
 		return err;
 
 	*f = d;
-	if (*f != d)
-		return arg_bad("'%s' is out of range", arg);
+
+	/*allow true infinity via --foo=INF, while avoiding isinf() from math.h
+	  because it wasn't standard 25 years ago.*/
+	double inf = 1e300 * 1e300; /*direct 1e600 annoys -Woverflow*/
+	if ((d > FLT_MAX || d < -FLT_MAX) && d != inf && d != -inf)
+		return arg_bad("'%s' is out of range for a 32 bit float", arg);
+	if (d != 0 && *f == 0)
+		return arg_bad("'%s' is out of range (truncated to zero)", arg);
 
 	return NULL;
 }
diff --git a/ccan/opt/test/run-helpers.c b/ccan/opt/test/run-helpers.c
index 49fb206..6ec17f5 100644
--- a/ccan/opt/test/run-helpers.c
+++ b/ccan/opt/test/run-helpers.c
@@ -5,6 +5,7 @@
 #include <stdlib.h>
 #include <limits.h>
 #include "utils.h"
+#include <math.h>
 
 /* We don't actually want it to exit... */
 static jmp_buf exited;
@@ -77,7 +78,7 @@ static void set_args(int *argc, char ***argv, ...)
 /* Test helpers. */
 int main(int argc, char *argv[])
 {
-	plan_tests(476);
+	plan_tests(500);
 
 	/* opt_set_bool */
 	{
@@ -215,9 +216,25 @@ int main(int argc, char *argv[])
 		ok1(arg == 9999);
 		ok1(parse_args(&argc, &argv, "-a", "-9999", NULL));
 		ok1(arg == -9999);
+		ok1(parse_args(&argc, &argv, "-a", "1e33", NULL));
+		ok1(arg == 1e33f);
+		/*overflows should fail */
+		ok1(!parse_args(&argc, &argv, "-a", "1e39", NULL));
+		ok1(!parse_args(&argc, &argv, "-a", "-1e40", NULL));
+		/*low numbers lose precision but work */
+		ok1(parse_args(&argc, &argv, "-a", "1e-39", NULL));
+		ok1(arg == 1e-39f);
+		ok1(parse_args(&argc, &argv, "-a", "-1e-45", NULL));
+		ok1(arg == -1e-45f);
+		ok1(!parse_args(&argc, &argv, "-a", "1e-99", NULL));
 		ok1(parse_args(&argc, &argv, "-a", "0", NULL));
 		ok1(arg == 0);
+		ok1(parse_args(&argc, &argv, "-a", "1.111111111111", NULL));
+		ok1(arg == 1.1111112f);
+		ok1(parse_args(&argc, &argv, "-a", "INF", NULL));
+		ok1(isinf(arg));
 		ok1(!parse_args(&argc, &argv, "-a", "100crap", NULL));
+		ok1(!parse_args(&argc, &argv, "-a", "1e7crap", NULL));
 	}
 	/* opt_set_doubleval */
 	{
@@ -228,9 +245,19 @@ int main(int argc, char *argv[])
 		ok1(arg == 9999);
 		ok1(parse_args(&argc, &argv, "-a", "-9999", NULL));
 		ok1(arg == -9999);
+		ok1(parse_args(&argc, &argv, "-a", "1e-299", NULL));
+		ok1(arg == 1e-299);
+		ok1(parse_args(&argc, &argv, "-a", "-1e-305", NULL));
+		ok1(arg == -1e-305);
+		ok1(!parse_args(&argc, &argv, "-a", "1e-499", NULL));
 		ok1(parse_args(&argc, &argv, "-a", "0", NULL));
 		ok1(arg == 0);
+		ok1(parse_args(&argc, &argv, "-a", "1.1111111111111111111", NULL));
+		ok1(arg == 1.1111111111111112);
+		ok1(parse_args(&argc, &argv, "-a", "INF", NULL));
+		ok1(isinf(arg));
 		ok1(!parse_args(&argc, &argv, "-a", "100crap", NULL));
+		ok1(!parse_args(&argc, &argv, "-a", "1e7crap", NULL));
 	}
 
 	{
-- 
1.9.1