Fix dangling pointer issue (#4786)

clang may not realize that memcpy() doesn't keep the pointers handed
to it around, so there's no risk of the address of an auto being kept
past the caller's return, hence the "dangling pointer" warning.

Instead, head it off at the pass by immediately returning if len
is zero, so in needn't be set to the address of an auto; the only
explicit passing of NULL for in passes 0 for len. (Comments explain
how if len == 0, nothing changes in the context.) One could argue
for checking in rather than or in addition to checking len... OTOH,
shouldn't passing NULL and a non-zero len break loudly?

diff --git a/src/lib/util/sha1.c b/src/lib/util/sha1.c
index da492d3ce6ed2ac235a41a7a8a5a860f9f6180aa..8bcaa6847d8b585f7fb04a2dea920079950857f8 100644 (file)
--- a/src/lib/util/sha1.c
+++ b/src/lib/util/sha1.c
@@ -107,12 +107,14 @@ void fr_sha1_update(fr_sha1_ctx *context, uint8_t const *in, size_t len)
        unsigned int i, j;
 
        /*
-        *      Needed so we can calculate the zero
-        *      length sha1 hash correctly.
-        *      ubsan doesn't like arithmetic on
-        *      NULL pointers.
+        *      If len == 0, there's nothing to do:
+        *      First, len == 0 impolies len * 8 == 0, so the contents of
+        *      context->count wouldn't change.
+        *      j by construction is <= 63, so if len == 0, j + len == j <= 63, and
+        *      i would be set to 0 and the final memcpy() would "copy" 0
+        *      bytes, so the contents of context->buffer wouldn't change either.
         */
-       if (!in) in = (uint8_t[]){ 0x00 };
+       if (len == 0) return;
 
        j = (context->count[0] >> 3) & 63;
        if ((context->count[0] += len << 3) < (len << 3)) {