[PATCH v3 0/1] MR7357: rpcrt4: Avoid unsigned overflows in buffer validation.

[Jacek Caban (＠jacek)]

Fixes Clang 20 `-Wtautological-compare` warnings. Based on patch by William Horvath.

--
  v3: rpcrt4: Don't validate buffer in NDR marshaler.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7357

[PATCH v3 1/1] rpcrt4: Don't validate buffer in NDR marshaler.

[Jacek Caban]

From: Jacek Caban <jacek(a)codeweavers.com>

---
 dlls/rpcrt4/ndr_marshall.c       | 62 ++++++++++++++------------------
 dlls/rpcrt4/tests/ndr_marshall.c | 33 +++++++++++++++++
 2 files changed, 60 insertions(+), 35 deletions(-)

diff --git a/dlls/rpcrt4/ndr_marshall.c b/dlls/rpcrt4/ndr_marshall.c
index c68eb0ee57e..60b44f27d48 100644
--- a/dlls/rpcrt4/ndr_marshall.c
+++ b/dlls/rpcrt4/ndr_marshall.c
@@ -746,16 +746,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)
+static inline void 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))
-    {
-        ERR("buffer overflow - Buffer = %p, BufferEnd = %p, size = %lu\n",
-            pStubMsg->Buffer, (unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength,
-            size);
-        RpcRaiseException(RPC_X_BAD_STUB_DATA);
-    }
     memcpy(pStubMsg->Buffer, p, size);
     pStubMsg->Buffer += size;
 }
@@ -1547,7 +1539,7 @@ unsigned char * WINAPI NdrPointerMarshall(PMIDL_STUB_MESSAGE pStubMsg,
   {
     align_pointer_clear(&pStubMsg->Buffer, 4);
     Buffer = pStubMsg->Buffer;
-    safe_buffer_increment(pStubMsg, 4);
+    pStubMsg->Buffer += 4;
   }
   else
     Buffer = pStubMsg->Buffer;
@@ -1749,7 +1741,7 @@ unsigned char * WINAPI NdrSimpleStructMarshall(PMIDL_STUB_MESSAGE pStubMsg,
   align_pointer_clear(&pStubMsg->Buffer, pFormat[1] + 1);
 
   pStubMsg->BufferMark = pStubMsg->Buffer;
-  safe_copy_to_buffer(pStubMsg, pMemory, size);
+  copy_to_buffer(pStubMsg, pMemory, size);
 
   if (pFormat[0] != FC_STRUCT)
     EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat+4);
@@ -2036,7 +2028,7 @@ static inline void array_write_variance_and_marshall(
     size = safe_multiply(esize, pStubMsg->MaxCount);
     if (fHasPointers)
       pStubMsg->BufferMark = pStubMsg->Buffer;
-    safe_copy_to_buffer(pStubMsg, pMemory, size);
+    copy_to_buffer(pStubMsg, pMemory, size);
 
     if (fHasPointers)
       EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat);
@@ -2056,7 +2048,7 @@ static inline void array_write_variance_and_marshall(
 
     if (fHasPointers)
       pStubMsg->BufferMark = pStubMsg->Buffer;
-    safe_copy_to_buffer(pStubMsg, pMemory + pStubMsg->Offset, size);
+    copy_to_buffer(pStubMsg, pMemory + pStubMsg->Offset, size);
 
     if (fHasPointers)
       EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat);
@@ -2071,7 +2063,7 @@ static inline void array_write_variance_and_marshall(
     WriteVariance(pStubMsg);
 
     size = safe_multiply(esize, pStubMsg->ActualCount);
-    safe_copy_to_buffer(pStubMsg, pMemory, size); /* the string itself */
+    copy_to_buffer(pStubMsg, pMemory, size); /* the string itself */
     break;
   case FC_BOGUS_ARRAY:
     alignment = pFormat[1] + 1;
@@ -2612,7 +2604,7 @@ unsigned char *  WINAPI NdrNonConformantStringMarshall(PMIDL_STUB_MESSAGE pStubM
   WriteVariance(pStubMsg);
 
   size = safe_multiply(esize, pStubMsg->ActualCount);
-  safe_copy_to_buffer(pStubMsg, pMemory, size); /* the string itself */
+  copy_to_buffer(pStubMsg, pMemory, size); /* the string itself */
 
   return NULL;
 }
@@ -2855,14 +2847,14 @@ static unsigned char * ComplexMarshall(PMIDL_STUB_MESSAGE pStubMsg,
     case FC_SMALL:
     case FC_USMALL:
       TRACE("byte=%d <= %p\n", *(WORD*)pMemory, pMemory);
-      safe_copy_to_buffer(pStubMsg, pMemory, 1);
+      copy_to_buffer(pStubMsg, pMemory, 1);
       pMemory += 1;
       break;
     case FC_WCHAR:
     case FC_SHORT:
     case FC_USHORT:
       TRACE("short=%d <= %p\n", *(WORD*)pMemory, pMemory);
-      safe_copy_to_buffer(pStubMsg, pMemory, 2);
+      copy_to_buffer(pStubMsg, pMemory, 2);
       pMemory += 2;
       break;
     case FC_ENUM16:
@@ -2871,7 +2863,7 @@ static unsigned char * ComplexMarshall(PMIDL_STUB_MESSAGE pStubMsg,
       TRACE("enum16=%ld <= %p\n", *(DWORD*)pMemory, pMemory);
       if (32767 < *(DWORD*)pMemory)
         RpcRaiseException(RPC_X_ENUM_VALUE_OUT_OF_RANGE);
-      safe_copy_to_buffer(pStubMsg, &val, 2);
+      copy_to_buffer(pStubMsg, &val, 2);
       pMemory += 4;
       break;
     }
@@ -2879,7 +2871,7 @@ static unsigned char * ComplexMarshall(PMIDL_STUB_MESSAGE pStubMsg,
     case FC_ULONG:
     case FC_ENUM32:
       TRACE("long=%ld <= %p\n", *(DWORD*)pMemory, pMemory);
-      safe_copy_to_buffer(pStubMsg, pMemory, 4);
+      copy_to_buffer(pStubMsg, pMemory, 4);
       pMemory += 4;
       break;
     case FC_INT3264:
@@ -2887,23 +2879,23 @@ static unsigned char * ComplexMarshall(PMIDL_STUB_MESSAGE pStubMsg,
     {
       UINT val = *(UINT_PTR *)pMemory;
       TRACE("int3264=%Id <= %p\n", *(UINT_PTR *)pMemory, pMemory);
-      safe_copy_to_buffer(pStubMsg, &val, sizeof(UINT));
+      copy_to_buffer(pStubMsg, &val, sizeof(UINT));
       pMemory += sizeof(UINT_PTR);
       break;
     }
     case FC_FLOAT:
       TRACE("float=%f <= %p\n", *(float*)pMemory, pMemory);
-      safe_copy_to_buffer(pStubMsg, pMemory, sizeof(float));
+      copy_to_buffer(pStubMsg, pMemory, sizeof(float));
       pMemory += sizeof(float);
       break;
     case FC_HYPER:
       TRACE("longlong=%s <= %p\n", wine_dbgstr_longlong(*(ULONGLONG*)pMemory), pMemory);
-      safe_copy_to_buffer(pStubMsg, pMemory, 8);
+      copy_to_buffer(pStubMsg, pMemory, 8);
       pMemory += 8;
       break;
     case FC_DOUBLE:
       TRACE("double=%f <= %p\n", *(double*)pMemory, pMemory);
-      safe_copy_to_buffer(pStubMsg, pMemory, sizeof(double));
+      copy_to_buffer(pStubMsg, pMemory, sizeof(double));
       pMemory += sizeof(double);
       break;
     case FC_RP:
@@ -4744,7 +4736,7 @@ unsigned char *  WINAPI NdrConformantStructMarshall(PMIDL_STUB_MESSAGE pStubMsg,
     }
     /* copy constant sized part of struct */
     pStubMsg->BufferMark = pStubMsg->Buffer;
-    safe_copy_to_buffer(pStubMsg, pMemory, pCStructFormat->memory_size + bufsize);
+    copy_to_buffer(pStubMsg, pMemory, pCStructFormat->memory_size + bufsize);
 
     if (pCStructFormat->type == FC_CPSTRUCT)
         EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat);
@@ -4950,7 +4942,7 @@ unsigned char *  WINAPI NdrConformantVaryingStructMarshall(PMIDL_STUB_MESSAGE pS
 
     /* write constant sized part */
     pStubMsg->BufferMark = pStubMsg->Buffer;
-    safe_copy_to_buffer(pStubMsg, pMemory, pCVStructFormat->memory_size);
+    copy_to_buffer(pStubMsg, pMemory, pCVStructFormat->memory_size);
 
     array_write_variance_and_marshall(*pCVArrayFormat, pStubMsg,
                                       pMemory + pCVStructFormat->memory_size,
@@ -5199,7 +5191,7 @@ unsigned char *  WINAPI NdrFixedArrayMarshall(PMIDL_STUB_MESSAGE pStubMsg,
     }
 
     pStubMsg->BufferMark = pStubMsg->Buffer;
-    safe_copy_to_buffer(pStubMsg, pMemory, total_size);
+    copy_to_buffer(pStubMsg, pMemory, total_size);
 
     pFormat = EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat);
 
@@ -5426,7 +5418,7 @@ unsigned char *  WINAPI NdrVaryingArrayMarshall(PMIDL_STUB_MESSAGE pStubMsg,
 
     bufsize = safe_multiply(esize, pStubMsg->ActualCount);
     pStubMsg->BufferMark = pStubMsg->Buffer;
-    safe_copy_to_buffer(pStubMsg, pMemory + pStubMsg->Offset, bufsize);
+    copy_to_buffer(pStubMsg, pMemory + pStubMsg->Offset, bufsize);
 
     EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat);
 
@@ -6626,14 +6618,14 @@ static unsigned char *WINAPI NdrBaseTypeMarshall(
     case FC_CHAR:
     case FC_SMALL:
     case FC_USMALL:
-        safe_copy_to_buffer(pStubMsg, pMemory, sizeof(UCHAR));
+        copy_to_buffer(pStubMsg, pMemory, sizeof(UCHAR));
         TRACE("value: 0x%02x\n", *pMemory);
         break;
     case FC_WCHAR:
     case FC_SHORT:
     case FC_USHORT:
         align_pointer_clear(&pStubMsg->Buffer, sizeof(USHORT));
-        safe_copy_to_buffer(pStubMsg, pMemory, sizeof(USHORT));
+        copy_to_buffer(pStubMsg, pMemory, sizeof(USHORT));
         TRACE("value: 0x%04x\n", *(USHORT *)pMemory);
         break;
     case FC_LONG:
@@ -6641,20 +6633,20 @@ static unsigned char *WINAPI NdrBaseTypeMarshall(
     case FC_ERROR_STATUS_T:
     case FC_ENUM32:
         align_pointer_clear(&pStubMsg->Buffer, sizeof(ULONG));
-        safe_copy_to_buffer(pStubMsg, pMemory, sizeof(ULONG));
+        copy_to_buffer(pStubMsg, pMemory, sizeof(ULONG));
         TRACE("value: 0x%08lx\n", *(ULONG *)pMemory);
         break;
     case FC_FLOAT:
         align_pointer_clear(&pStubMsg->Buffer, sizeof(float));
-        safe_copy_to_buffer(pStubMsg, pMemory, sizeof(float));
+        copy_to_buffer(pStubMsg, pMemory, sizeof(float));
         break;
     case FC_DOUBLE:
         align_pointer_clear(&pStubMsg->Buffer, sizeof(double));
-        safe_copy_to_buffer(pStubMsg, pMemory, sizeof(double));
+        copy_to_buffer(pStubMsg, pMemory, sizeof(double));
         break;
     case FC_HYPER:
         align_pointer_clear(&pStubMsg->Buffer, sizeof(ULONGLONG));
-        safe_copy_to_buffer(pStubMsg, pMemory, sizeof(ULONGLONG));
+        copy_to_buffer(pStubMsg, pMemory, sizeof(ULONGLONG));
         TRACE("value: %s\n", wine_dbgstr_longlong(*(ULONGLONG*)pMemory));
         break;
     case FC_ENUM16:
@@ -6664,7 +6656,7 @@ static unsigned char *WINAPI NdrBaseTypeMarshall(
         if (*(UINT *)pMemory > SHRT_MAX)
             RpcRaiseException(RPC_X_ENUM_VALUE_OUT_OF_RANGE);
         align_pointer_clear(&pStubMsg->Buffer, sizeof(USHORT));
-        safe_copy_to_buffer(pStubMsg, &val, sizeof(val));
+        copy_to_buffer(pStubMsg, &val, sizeof(val));
         TRACE("value: 0x%04x\n", *(UINT *)pMemory);
         break;
     }
@@ -6673,7 +6665,7 @@ static unsigned char *WINAPI NdrBaseTypeMarshall(
     {
         UINT val = *(UINT_PTR *)pMemory;
         align_pointer_clear(&pStubMsg->Buffer, sizeof(UINT));
-        safe_copy_to_buffer(pStubMsg, &val, sizeof(val));
+        copy_to_buffer(pStubMsg, &val, sizeof(val));
         break;
     }
     case FC_IGNORE:
diff --git a/dlls/rpcrt4/tests/ndr_marshall.c b/dlls/rpcrt4/tests/ndr_marshall.c
index 0281dd241f6..b30750fca4c 100644
--- a/dlls/rpcrt4/tests/ndr_marshall.c
+++ b/dlls/rpcrt4/tests/ndr_marshall.c
@@ -2999,6 +2999,38 @@ static void test_NdrCorrelationInitialize(void)
     ok( stub_msg.CorrDespIncrement == 1, "got %d\n", stub_msg.CorrDespIncrement );
 }
 
+static void test_pointer_validation(void)
+{
+    MIDL_STUB_DESC desc = Object_StubDesc;
+    RPC_MESSAGE rpc_msg;
+    MIDL_STUB_MESSAGE msg;
+    LONG l = 1;
+    unsigned char buf[32];
+
+    static const unsigned char format[] =
+    {
+        0x12, 0x8,      /* FC_UP [simple_pointer] */
+        0x8,            /* FC_LONG */
+        0x5c,           /* FC_PAD */
+    };
+
+    desc.pFormatTypes = format;
+    NdrClientInitializeNew( &rpc_msg, &msg, &desc, 0 );
+
+    /* Pass invalid buffer pointers to check that they are not validated */
+    msg.RpcMsg->Buffer = msg.BufferStart = (void *)0xdeadbeef;
+
+    msg.BufferLength = 1;
+    msg.Buffer = buf;
+    NdrPointerMarshall( &msg, (unsigned char*)&l, format );
+    ok( msg.Buffer == buf + 8, "unexpected Buffer %p, expected %p\n", msg.Buffer, msg.BufferStart );
+
+    msg.BufferLength = ~0u;
+    msg.Buffer = buf;
+    NdrPointerMarshall( &msg, (unsigned char*)&l, format );
+    ok( msg.Buffer == buf + 8, "unexpected Buffer %p, expected %p\n", msg.Buffer, msg.BufferStart );
+}
+
 START_TEST( ndr_marshall )
 {
     determine_pointer_marshalling_style();
@@ -3023,4 +3055,5 @@ START_TEST( ndr_marshall )
     test_NdrGetUserMarshalInfo();
     test_MesEncodeFixedBufferHandleCreate();
     test_NdrCorrelationInitialize();
+    test_pointer_validation();
 }
-- 
GitLab

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

[PATCH v3 0/1] MR7357: rpcrt4: Avoid unsigned overflows in buffer validation. - approved

[Huw Davies (＠huw)]

This merge request was approved by Huw Davies.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7357