From: James Jones <jejones3141@gmail.com>
Date: Tue, 25 Oct 2022 22:22:44 +0000 (-0500)
Subject: Fix dangling pointer issue (#4786)
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=332f13652bfbe99dcd0e5221bdd6c5bb1cc24c35;p=thirdparty%2Ffreeradius-server.git

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 da492d3ce6e..8bcaa6847d8 100644
--- 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)) {