Fix an undefined behaviour in os/rand.c

[jkbonfield]

This is really picky and I almost didn't bother given this is just code lifted from FreeBSD which hasn't changed it in 32 years.

https://github.com/lattera/freebsd/blame/401a161083850a9a4ce916f37520c084cff1543b/lib/libc/gen/_rand48.c#L32-L49

However undefined behaviour sanitizer whinges about signed overflows. This happens because the promotion of unsigned short in arithmetic expressions is to signed int, and then multiplying signed ints can overflow from signed to unsigned.

Eg:

    (gdb) p (unsigned short)50000 * (unsigned short)45000
    $1 = -2044967296
    (gdb) p (unsigned int)50000 * (unsigned int)45000
    $2 = 2250000000

Due to 2s complement this makes no difference and casting both to unsigned int before the multiplcation gives you the same bit pattern. It's just the resulting type that differs, as we change that in the assignment anyway.

(Note there are similar undefined behaviour issues with md5.c, which I didn't fix.)

[daviesrob]

You missed the cast on xseed[0], although it shouldn't matter as the type promotion should still work. It would be nice to be consistent though.

I do wonder if it would be better to declare _rand48_mult and _rand48_add as unsigned long? Then almost all the casts could be removed, leaving just those that truncate accu to unsigned short. I guess 32 years ago saving the memory on the tables was useful, but we could probably afford the extra space for the bigger types now.

[jkbonfield]

Verified the change with a quick test main function:

#include <stdio.h>
int main(void) {
        hts_srand48(15551);
        for (int i = 0; i < 1000; i++)
                printf("%ld\n", hts_lrand48());
        return 0;
}

This allows us to compile locally and get the warning:

$ gcc16 -DHTSLIB_EXPORT= -g -O3 -DHTSLIB_EXPORT= -fsanitize=undefined -I.. -I../htslib rand.c && ./a.out |md5sum
rand.c:59:55: runtime error: signed integer overflow: 57068 * 55855 cannot be represented in type 'int'
rand.c:59:26: runtime error: signed integer overflow: 58989 * 54801 cannot be represented in type 'int'
rand.c:59:37: runtime error: signed integer overflow: -1730715466 + -567742080 cannot be represented in type 'int'
791ba4ef8d459f5c99a75dfc7c6bee6f  -

I switched to using long multi/add variables, and removed all the now unnecessary casts from that function (which were there in the original version too).

[jkbonfield]

I can't reproduce the md5.c I claimed above. I obviously saw it in one of my CI tests, but it's gone now anyway. For future investigation, my test harness was:

diff --git a/md5.c b/md5.c
index 1a43da50..8f705152 100644
--- a/md5.c
+++ b/md5.c
@@ -53,6 +53,10 @@
 #include "htslib/hts.h"
 #include "htslib/hts_endian.h"
 
+#ifdef TEST_MAIN
+#  undef HAVE_OPENSSL
+#endif
+
 #ifndef HAVE_OPENSSL
 
 #include <string.h>
@@ -386,3 +390,24 @@ void hts_md5_hex(char *hex, const unsigned char *digest)
     }
     hex[32] = 0;
 }
+
+#ifdef TEST_MAIN
+#include <stdio.h>
+int main(void) {
+    char buf[15551], hex[33];
+    unsigned char bin[16];
+
+    srand(1);
+    for (int i = 0; i < 15551; i++)
+       buf[i] = rand();
+
+    hts_md5_context *md5 = hts_md5_init();
+    hts_md5_update(md5, buf, 15551);
+    hts_md5_final(bin, md5);
+    hts_md5_hex(hex, bin);
+    hts_md5_destroy(md5);
+    puts(hex);
+
+    return 0;
+}
+#endif

(Verified it's not using openssl code.)

It's irrelevant to this PR anyway, but we can shelve the md5.c possible issue until we come across it again, if ever. (It's been years without problem so far.)

[daviesrob]

Looks tidier, but i think you should keep the casts to unsigned short. Such narrowing conversions can become noisy if -Wconversion is in use.

[jkbonfield]

Looks tidier, but i think you should keep the casts to unsigned short. Such narrowing conversions can become noisy if -Wconversion is in use.

Added them back then. (Also bah humbug on annoying spammy warnings! Although even -Wextra doesn't complain.)