[PATCH v4 0/1] MR7231: rpcrt4: Change some buffer wrap checks to satisfy Clang 20.
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
From: William Horvath <william(a)horvath.blog> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7231
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7231#note_95009
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7231#note_95013
This merge request was closed by William Horvath. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7231
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).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7231#note_95014
participants (3)
-
Jacek Caban (＠jacek) -
William Horvath
-
William Horvath (＠whrvt)