opt/helpers: fix out-of-range check in opt_set_floatval()

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@halo.gen.nz>

diff --git a/ccan/opt/helpers.c b/ccan/opt/helpers.c
index 43b86d7ca2ad953665522cf37f449b8046ffd3b7..f247301cd7a2ba236871f8327c56a79ea144509f 100644 (file)
--- 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 49fb2062e30427725c4b79b33303df55edc02e14..6ec17f589fdb76d8c1fc7f0f78d39619d18c6294 100644 (file)
--- 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));
        }
 
        {