From 0663c4e9200ecd06ca79eb49d7b2c94b0484433c Mon Sep 17 00:00:00 2001
From: Donald Buczek <buczek@molgen.mpg.de>
Date: Sun, 1 May 2022 20:48:35 +0200
Subject: [PATCH] mx_util: Avoid "may be used uninitialized" warning

The function

    int mx_strtoul(char *str, unsigned long int *to)

and three similar functions either returns with the success value 0 or
with a negative error code (-errno). The output argument `to` is only
written, when the return value signals success. If the function returns
with a negative value, the caller can not assument that `to` has been
set.

When a caller does something like this

    int value;
    res = int mx_strtoul(str, &value);
    if (res < 0)
        return;
    do_something_with(value)

everything is correct according to the description aboce.

The code in mx_strtoul has this pattern:

    errno = 0;
    ul = strtoul(str, &end, 0);
    if (errno) {
        return -errno;
    }
    *to = ul;

When the caller is in the same compilation unit, the compiler (gcc with
-O2 or higher and with -Wall) might raise a "may be used uninitialized"
warning for the output variable (`value` in the above example).

The problem here is, that the caller assumes a negative value for an
error status but the callee returns -errno for any errno value other
than zero which might include negative errno values. We know, that the
errno value will not be negative because we've set it to zero and the
library function will conditionally set it to a valid error number only,
which is alway positiv[1]. But the compiler doesn't share this
assumption and takes a negative errno value into account.

When the caller and the callee are in the same compilation unit, the
interprocedural optimizer might detect the code path with a negative
errno value which would in fact trigger a usage of an unitilialized
value.

We can fix that by just baking the assumption into the comparison:

    if (errno > 0)
        return -errno;

or by asserting that errno is not negative:

    if (errno) {
        assert(errno > 0);
        return -errno;
    }

or by defining and using an __assume macro:

    #if defined(__clang__)
    #define assume(cond) __builtin_assume(cond)
    #elif defined(__GNUC__)
    #define assume(cond) do { if (!(cond)) __builtin_unreachable(); } while (0)
    #endif

    ...

    if (errno) {
        assume(errno > 0);
        return -errno;
    }

Use the first option for all four affected functions.

Note, that clangs static analyzer is still not happy with this, but
this is probably a false positive of the analyzer [2].

[1]: errno(3): "Valid error numbers are all positive numbers.
[2]: https://github.com/llvm/llvm-project/issues/55241
---
 mx_util.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mx_util.c b/mx_util.c
index 48718e56..bc472ab0 100644
--- a/mx_util.c
+++ b/mx_util.c
@@ -256,7 +256,7 @@ int mx_strtoul(char *str, unsigned long int *to)
 
     ul = strtoul(str, &end, 0);
 
-    if (errno)
+    if (errno > 0)
         return -errno;
 
     end = mx_strskipwhitespaces(end);
@@ -284,7 +284,7 @@ int mx_strtoull(char *str, unsigned long long int *to)
 
     ull = strtoull(str, &end, 0);
 
-    if (errno)
+    if (errno > 0)
         return -errno;
 
     end = mx_strskipwhitespaces(end);
@@ -314,7 +314,7 @@ int mx_strtol(char *str, signed long int *to)
 
     l = strtoul(str, &end, 0);
 
-    if (errno)
+    if (errno > 0)
         return -errno;
 
     end = mx_strskipwhitespaces(end);
@@ -339,7 +339,7 @@ int mx_strtoll(char *str, signed long long int *to)
 
     ll = strtoll(str, &end, 0);
 
-    if (errno)
+    if (errno > 0)
         return -errno;
 
     end = mx_strskipwhitespaces(end);