From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>,
        Thomas Munro <thomas.munro@gmail.com>
Subject: Re: Pointer subtraction with a null pointer
In-reply-to: <20220326174953.gdeasrl5wzal42ic@alap3.anarazel.de>
References: <48826.1648310694@sss.pgh.pa.us> <49360.1648311192@sss.pgh.pa.us>
 <20220326163441.qzpcioycozbzarsf@alap3.anarazel.de>
 <51678.1648312664@sss.pgh.pa.us> <55128.1648315414@sss.pgh.pa.us>
 <20220326174953.gdeasrl5wzal42ic@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Sat, 26 Mar 2022 10:49:53 -0700"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <103039.1648318377.0@sss.pgh.pa.us>
Date: Sat, 26 Mar 2022 14:13:56 -0400
Message-ID: <103098.1648318436@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/103098.1648318436%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <103039.1648318377.1@sss.pgh.pa.us>

Andres Freund <andres@anarazel.de> writes:
> Wonder if we should try to get rid of the problem by also fixing the double
> evaluation of val? I think something like

Good idea.  The attached also silences the warning, and getting rid
of the double-eval hazard seems like a net win.

> Might also be worth adding an assertion that base < val.

Did that too.  On the whole I like this better.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="silence-null-subtraction-warning-2.patch";
	charset="us-ascii"
Content-ID: <103039.1648318377.2@sss.pgh.pa.us>
Content-Description: silence-null-subtraction-warning-2.patch

diff --git a/src/include/utils/relptr.h b/src/include/utils/relptr.h
index fdc2124d2c..cc80a7200d 100644
--- a/src/include/utils/relptr.h
+++ b/src/include/utils/relptr.h
@@ -56,11 +56,24 @@
 #define relptr_is_null(rp) \
 	((rp).relptr_off == 0)
 
+/* We use this inline to avoid double eval of "val" in relptr_store */
+static inline Size
+relptr_store_eval(char *base, char *val)
+{
+	if (val == NULL)
+		return 0;
+	else
+	{
+		Assert(val > base);
+		return val - base;
+	}
+}
+
 #ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P
 #define relptr_store(base, rp, val) \
 	(AssertVariableIsOfTypeMacro(base, char *), \
 	 AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \
-	 (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
+	 (rp).relptr_off = relptr_store_eval(base, (char *) (val)))
 #else
 /*
  * If we don't have __builtin_types_compatible_p, assume we might not have
@@ -68,7 +81,7 @@
  */
 #define relptr_store(base, rp, val) \
 	(AssertVariableIsOfTypeMacro(base, char *), \
-	 (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
+	 (rp).relptr_off = relptr_store_eval(base, (char *) (val)))
 #endif
 
 #define relptr_copy(rp1, rp2) \

------- =_aaaaaaaaaa0--