[PATCH v4 0/1] MR7231: rpcrt4: Change some buffer wrap checks to satisfy Clang 20.

[William Horvath (＠whrvt)]

Soon-to-be-released Clang 20 introduced a [`-Wtautological-compare`](https://github.com/llvm/llvm-project/commit/6d34cfac53b993a6cdf3d6669e017eac...) warning which these checks trigger. Update them in a similar fashion to the example shown in the [Clang doc](https://github.com/llvm/llvm-project/blob/release/20.x/clang/docs/ReleaseNot...) (see "To avoid pointer addition overflow...").

--
  v4: rpcrt4: Pointer arithmetic fixes (-Wtautological-compare).

https://gitlab.winehq.org/wine/wine/-/merge_requests/7231

[PATCH v4 1/1] rpcrt4: Pointer arithmetic fixes (-Wtautological-compare).

[William Horvath]

From: William Horvath

---
 dlls/rpcrt4/ndr_marshall.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/dlls/rpcrt4/ndr_marshall.c b/dlls/rpcrt4/ndr_marshall.c
index c68eb0ee57e..8f44fa66144 100644
--- a/dlls/rpcrt4/ndr_marshall.c
+++ b/dlls/rpcrt4/ndr_marshall.c
@@ -711,8 +711,8 @@ static inline ULONG safe_multiply(ULONG a, ULONG b)

static inline void safe_buffer_increment(MIDL_STUB_MESSAGE *pStubMsg, ULONG size)
 {
-    if ((pStubMsg->Buffer + size < pStubMsg->Buffer) || /* integer overflow of pStubMsg->Buffer */
-        (pStubMsg->Buffer + size > (unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength))
+    if (((unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength) < pStubMsg->Buffer || /* integer overflow of pStubMsg->Buffer */
+        size > ((unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength) - pStubMsg->Buffer)
         RpcRaiseException(RPC_X_BAD_STUB_DATA);
     pStubMsg->Buffer += size;
 }
@@ -732,8 +732,8 @@ static inline void safe_buffer_length_increment(MIDL_STUB_MESSAGE *pStubMsg, ULO
  * to do so */
 static inline void safe_copy_from_buffer(MIDL_STUB_MESSAGE *pStubMsg, void *p, ULONG size)
 {
-    if ((pStubMsg->Buffer + size < pStubMsg->Buffer) || /* integer overflow of pStubMsg->Buffer */
-        (pStubMsg->Buffer + size > pStubMsg->BufferEnd))
+    if (pStubMsg->BufferEnd < pStubMsg->Buffer || /* integer overflow of pStubMsg->Buffer */
+        size > (pStubMsg->BufferEnd - pStubMsg->Buffer))
     {
         ERR("buffer overflow - Buffer = %p, BufferEnd = %p, size = %lu\n",
             pStubMsg->Buffer, pStubMsg->BufferEnd, size);
@@ -748,8 +748,8 @@ static inline void safe_copy_from_buffer(MIDL_STUB_MESSAGE *pStubMsg, void *p, U
 /* copies data to the buffer, checking that there is enough space to do so */
 static inline void safe_copy_to_buffer(MIDL_STUB_MESSAGE *pStubMsg, const void *p, ULONG size)
 {
-    if ((pStubMsg->Buffer + size < pStubMsg->Buffer) || /* integer overflow of pStubMsg->Buffer */
-        (pStubMsg->Buffer + size > (unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength))
+    if (((unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength) < pStubMsg->Buffer || /* integer overflow of pStubMsg->Buffer */
+        size > ((unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength) - pStubMsg->Buffer)
     {
         ERR("buffer overflow - Buffer = %p, BufferEnd = %p, size = %lu\n",
             pStubMsg->Buffer, (unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength,
@@ -767,8 +767,8 @@ static void validate_string_data(MIDL_STUB_MESSAGE *pStubMsg, ULONG bufsize, ULO
     ULONG i;

/* verify the buffer is safe to access */
-    if ((pStubMsg->Buffer + bufsize < pStubMsg->Buffer) ||
-        (pStubMsg->Buffer + bufsize > pStubMsg->BufferEnd))
+    if (pStubMsg->BufferEnd < pStubMsg->Buffer ||
+        bufsize > (pStubMsg->BufferEnd - pStubMsg->Buffer))
     {
         ERR("bufsize 0x%lx exceeded buffer end %p of buffer %p\n", bufsize,
             pStubMsg->BufferEnd, pStubMsg->Buffer);

[Jacek Caban (＠jacek)]

I simplified those checks a bit, see https://gitlab.winehq.org/jacek/wine/-/commit/dc54ab032c078b475d6d6b00c30f3b.... Please update MR with that if it looks good to you.

[William Horvath (＠whrvt)]

On Tue Feb 18 13:41:56 2025 +0000, Jacek Caban wrote:
> I simplified those checks a bit, see
> https://gitlab.winehq.org/jacek/wine/-/commit/dc54ab032c078b475d6d6b00c30f3b....
> Please update MR with that if it looks good to you.

I can't deny that it looks cleaner, but I feel very guilty for being kept as the author of that. I don't mind taking suggestions, but if you decide that not a single line that I wrote is worth keeping (incl. commit message) I'll take the hint that I'm just getting in the way here.

Feel free to open another merge request, sorry for wasting your time.

[PATCH v4 0/1] MR7231: rpcrt4: Change some buffer wrap checks to satisfy Clang 20. - closed

[William Horvath (＠whrvt)]

This merge request was closed by William Horvath.

[Jacek Caban (＠jacek)]

On Tue Feb 18 13:41:56 2025 +0000, William Horvath wrote:
> I can't deny that it looks cleaner, but I feel very guilty for being
> kept as the author of that. I don't mind taking suggestions, but if you
> decide that not a single line that I wrote is worth keeping (incl.
> commit message) I'll take the hint that I'm just getting in the way here.
> Feel free to open another merge request, sorry for wasting your time.

I'm sorry you feel that way, that was not my intention at all, nor is it how I see it. The logic of my version was the same as yours, which is what really matters; I just made minor tweaks (just me nitpicking, really).