GCC12.2 warns about dereferencing a pointer to RpcPktHdr while it has been allocated to the size of one of the packet (hence smaller in some cases).
/home/eric/work/wine/dlls/rpcrt4/rpc_message.c:111:26: warning: array subscript 'RpcPktHdr[0]' is partly outside array bounds of 'unsigned char[24]' [-Warray-bounds] 111 | Header->common.rpc_ver = RPC_VER_MAJOR;
This patch fixes the warnings by accessing the created object through a pointer to their type (and not through the union).
Notes: - the 'max(sizeof(...), FIELD_OFFSET(...))' thingie in RPCRT4_BuildBindNackHeader avoids also a warning as the FIELD_OFFSET() can be smaller than the size of the structure. This could be avoided by using a flexible array member for the 'protocols' field instead of 'protocols[ANYSIZE_ARRAY]'. - I only changed the allocation routines when the allocated size is smaller than the union. If the strategy is validated, one could consider applying the same allocation strategy to all helpers for symmetry reasons.
Hence the draft status for now, waiting for feedback.
Signed-off-by: Eric Pouech eric.pouech@gmail.com
From: Eric Pouech eric.pouech@gmail.com
GCC12.2 warns about dereferencing a pointer to RpcPktHdr while it has been allocated to the size of one of the packet (hence smaller in some cases).
/home/eric/work/wine/dlls/rpcrt4/rpc_message.c:111:26: warning: array subscript 'RpcPktHdr[0]' is partly outside array bounds of 'unsigned char[24]' [-Warray-bounds] 111 | Header->common.rpc_ver = RPC_VER_MAJOR;
This patch fixes the warnings by accessing the created object through a pointer to their type (and not through the union).
Notes: - the 'max(sizeof(...), FIELD_OFFSET(...))' thingie in RPCRT4_BuildBindNackHeader avoids also a warning as the FIELD_OFFSET() can be smaller than the size of the structure. This could be avoided by using a flexible array member for the 'protocols' field instead of 'protocols[ANYSIZE_ARRAY]'. - I only changed the allocation routines when the allocated size is smaller than the union. If the strategy is validated, one could consider applying the same allocation strategy to all helpers for symmetry reasons.
Hence the draft status for now, waiting for feedback.
Signed-off-by: Eric Pouech eric.pouech@gmail.com --- dlls/rpcrt4/rpc_message.c | 73 +++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 37 deletions(-)
diff --git a/dlls/rpcrt4/rpc_message.c b/dlls/rpcrt4/rpc_message.c index 4a6ebf434f2..4df33b6f3cc 100644 --- a/dlls/rpcrt4/rpc_message.c +++ b/dlls/rpcrt4/rpc_message.c @@ -105,21 +105,21 @@ static BOOL packet_does_auth_negotiation(const RpcPktHdr *Header) } }
-static VOID RPCRT4_BuildCommonHeader(RpcPktHdr *Header, unsigned char PacketType, +static VOID RPCRT4_BuildCommonHeader(RpcPktCommonHdr *Header, unsigned char PacketType, ULONG DataRepresentation) { - Header->common.rpc_ver = RPC_VER_MAJOR; - Header->common.rpc_ver_minor = RPC_VER_MINOR; - Header->common.ptype = PacketType; - Header->common.drep[0] = LOBYTE(LOWORD(DataRepresentation)); - Header->common.drep[1] = HIBYTE(LOWORD(DataRepresentation)); - Header->common.drep[2] = LOBYTE(HIWORD(DataRepresentation)); - Header->common.drep[3] = HIBYTE(HIWORD(DataRepresentation)); - Header->common.auth_len = 0; - Header->common.call_id = 1; - Header->common.flags = 0; + Header->rpc_ver = RPC_VER_MAJOR; + Header->rpc_ver_minor = RPC_VER_MINOR; + Header->ptype = PacketType; + Header->drep[0] = LOBYTE(LOWORD(DataRepresentation)); + Header->drep[1] = HIBYTE(LOWORD(DataRepresentation)); + Header->drep[2] = LOBYTE(HIWORD(DataRepresentation)); + Header->drep[3] = HIBYTE(HIWORD(DataRepresentation)); + Header->auth_len = 0; + Header->call_id = 1; + Header->flags = 0; /* Flags and fragment length are computed in RPCRT4_Send. */ -} +}
static RpcPktHdr *RPCRT4_BuildRequestHeader(ULONG DataRepresentation, ULONG BufferLength, @@ -137,7 +137,7 @@ static RpcPktHdr *RPCRT4_BuildRequestHeader(ULONG DataRepresentation, return NULL; }
- RPCRT4_BuildCommonHeader(header, PKT_REQUEST, DataRepresentation); + RPCRT4_BuildCommonHeader(&header->common, PKT_REQUEST, DataRepresentation); header->common.frag_len = sizeof(header->request); header->request.alloc_hint = BufferLength; header->request.context_id = 0; @@ -153,18 +153,18 @@ static RpcPktHdr *RPCRT4_BuildRequestHeader(ULONG DataRepresentation,
RpcPktHdr *RPCRT4_BuildResponseHeader(ULONG DataRepresentation, ULONG BufferLength) { - RpcPktHdr *header; + RpcPktResponseHdr *header;
- header = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(header->response)); + header = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*header)); if (header == NULL) { return NULL; }
- RPCRT4_BuildCommonHeader(header, PKT_RESPONSE, DataRepresentation); - header->common.frag_len = sizeof(header->response); - header->response.alloc_hint = BufferLength; + RPCRT4_BuildCommonHeader(&header->common, PKT_RESPONSE, DataRepresentation); + header->common.frag_len = sizeof(*header); + header->alloc_hint = BufferLength;
- return header; + return (RpcPktHdr*)header; }
RpcPktHdr *RPCRT4_BuildFaultHeader(ULONG DataRepresentation, RPC_STATUS Status) @@ -176,7 +176,7 @@ RpcPktHdr *RPCRT4_BuildFaultHeader(ULONG DataRepresentation, RPC_STATUS Status) return NULL; }
- RPCRT4_BuildCommonHeader(header, PKT_FAULT, DataRepresentation); + RPCRT4_BuildCommonHeader(&header->common, PKT_FAULT, DataRepresentation); header->common.frag_len = sizeof(header->fault); header->fault.status = Status;
@@ -200,7 +200,7 @@ RpcPktHdr *RPCRT4_BuildBindHeader(ULONG DataRepresentation, } ctxt_elem = (RpcContextElement *)(&header->bind + 1);
- RPCRT4_BuildCommonHeader(header, PKT_BIND, DataRepresentation); + RPCRT4_BuildCommonHeader(&header->common, PKT_BIND, DataRepresentation); header->common.frag_len = sizeof(header->bind) + FIELD_OFFSET(RpcContextElement, transfer_syntaxes[1]); header->bind.max_tsize = MaxTransmissionSize; header->bind.max_rsize = MaxReceiveSize; @@ -215,17 +215,16 @@ RpcPktHdr *RPCRT4_BuildBindHeader(ULONG DataRepresentation,
static RpcPktHdr *RPCRT4_BuildAuthHeader(ULONG DataRepresentation) { - RpcPktHdr *header; + RpcPktAuth3Hdr *header;
- header = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, - sizeof(header->auth3)); + header = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*header)); if (header == NULL) return NULL;
- RPCRT4_BuildCommonHeader(header, PKT_AUTH3, DataRepresentation); - header->common.frag_len = sizeof(header->auth3); + RPCRT4_BuildCommonHeader(&header->common, PKT_AUTH3, DataRepresentation); + header->common.frag_len = sizeof(*header);
- return header; + return (RpcPktHdr*)header; }
RpcPktHdr *RPCRT4_BuildBindNackHeader(ULONG DataRepresentation, @@ -233,21 +232,21 @@ RpcPktHdr *RPCRT4_BuildBindNackHeader(ULONG DataRepresentation, unsigned char RpcVersionMinor, unsigned short RejectReason) { - RpcPktHdr *header; + RpcPktBindNAckHdr *header; + header = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, max(sizeof(*header), FIELD_OFFSET(RpcPktBindNAckHdr, protocols[1])));
- header = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, FIELD_OFFSET(RpcPktHdr, bind_nack.protocols[1])); if (header == NULL) { return NULL; }
- RPCRT4_BuildCommonHeader(header, PKT_BIND_NACK, DataRepresentation); + RPCRT4_BuildCommonHeader(&header->common, PKT_BIND_NACK, DataRepresentation); header->common.frag_len = FIELD_OFFSET(RpcPktHdr, bind_nack.protocols[1]); - header->bind_nack.reject_reason = RejectReason; - header->bind_nack.protocols_count = 1; - header->bind_nack.protocols[0].rpc_ver = RpcVersion; - header->bind_nack.protocols[0].rpc_ver_minor = RpcVersionMinor; + header->reject_reason = RejectReason; + header->protocols_count = 1; + header->protocols[0].rpc_ver = RpcVersion; + header->protocols[0].rpc_ver_minor = RpcVersionMinor;
- return header; + return (RpcPktHdr*)header; }
RpcPktHdr *RPCRT4_BuildBindAckHeader(ULONG DataRepresentation, @@ -272,7 +271,7 @@ RpcPktHdr *RPCRT4_BuildBindAckHeader(ULONG DataRepresentation, return NULL; }
- RPCRT4_BuildCommonHeader(header, PKT_BIND_ACK, DataRepresentation); + RPCRT4_BuildCommonHeader(&header->common, PKT_BIND_ACK, DataRepresentation); header->common.frag_len = header_size; header->bind_ack.max_tsize = MaxTransmissionSize; header->bind_ack.max_rsize = MaxReceiveSize; @@ -301,7 +300,7 @@ RpcPktHdr *RPCRT4_BuildHttpHeader(ULONG DataRepresentation, return NULL; }
- RPCRT4_BuildCommonHeader(header, PKT_HTTP, DataRepresentation); + RPCRT4_BuildCommonHeader(&header->common, PKT_HTTP, DataRepresentation); /* since the packet isn't current sent using RPCRT4_Send, set the flags * manually here */ header->common.flags = RPC_FLG_FIRST|RPC_FLG_LAST;
This looks ok to me. Moving the `protocols` array to a variable array is fine. I haven't looked fully, but would getting rid of `RpcPktHdr` in favour of using `RpcPktCommonHeader` reduce the number of casts?
On Mon Dec 5 17:11:11 2022 +0000, Huw Davies wrote:
This looks ok to me. Moving the `protocols` array to a variable array is fine. I haven't looked fully, but would getting rid of `RpcPktHdr` in favour of using `RpcPktCommonHeader` reduce the number of casts?
looking a bit deeper into it: - for the FAM field 'protocols': is there a good coverage of BindNAck packets in tests/? from perusing the code, this change would move the protocols[] array from header to payload. Header + payload should be correctly aggregated afterwards, but I would be better off if we could test it. I don't seem to find any test for that in regression tests. Since, the packets crafted locally only fill one protocol, using simply size of struct for allocation would do for now. I'm a bit afraid of changing this right now. - by 'getting rid of RpcPktHdr' you mean completely dropping the union? something like that?[rpcrt4-nounion.patch](/uploads/dda6c2f3fee7748d0933dcb4b0bf3f2b/rpcrt4-nounion.patch) - If so that's not a small change: ``` <<diffstat>> dlls/rpcrt4/rpc_assoc.c | 20 +-- dlls/rpcrt4/rpc_binding.h | 6 +- dlls/rpcrt4/rpc_defs.h | 24 ++-- dlls/rpcrt4/rpc_message.c | 312 +++++++++++++++++++++++--------------------- dlls/rpcrt4/rpc_message.h | 36 ++--- dlls/rpcrt4/rpc_server.c | 32 ++--- dlls/rpcrt4/rpc_transport.c | 73 ++++++----- 7 files changed, 259 insertions(+), 244 deletions(-) ``` and it adds more casts as the previous patch as we need to cast down from CommonHdr to another Hdr. I tried to hide them thanks to rpc_castdown (ugly ;-) macro (could rename RpcPktCommonHdr into RpcPktHdr to safe a few changes, but still).
There's still some uglyness in rpcrt4_http_prepare_out_pipe where getting rid of the union doesn't allow to predefine size of incoming packet... setting the type of pkt_from_server to HttpHdr hides the dust under the carpet, but IMO this should rather be dynamically allocated (like what's done in RPCRT4_default_receive_fragment) / (attached patch is wrong has it will write outside of local variable)
so I'd rather keep in simple for now (no FAM, keep the union(s)!!)
On Mon Dec 5 17:11:11 2022 +0000, eric pouech wrote:
looking a bit deeper into it:
- for the FAM field 'protocols': is there a good coverage of BindNAck
packets in tests/? from perusing the code, this change would move the protocols[] array from header to payload. Header + payload should be correctly aggregated afterwards, but I would be better off if we could test it. I don't seem to find any test for that in regression tests. Since, the packets crafted locally only fill one protocol, using simply size of struct for allocation would do for now. I'm a bit afraid of changing this right now.
- by 'getting rid of RpcPktHdr' you mean completely dropping the union?
something like that?[rpcrt4-nounion.patch](/uploads/dda6c2f3fee7748d0933dcb4b0bf3f2b/rpcrt4-nounion.patch)
- If so that's not a small change:
<<diffstat>> dlls/rpcrt4/rpc_assoc.c | 20 +-- dlls/rpcrt4/rpc_binding.h | 6 +- dlls/rpcrt4/rpc_defs.h | 24 ++-- dlls/rpcrt4/rpc_message.c | 312 +++++++++++++++++++++++--------------------- dlls/rpcrt4/rpc_message.h | 36 ++--- dlls/rpcrt4/rpc_server.c | 32 ++--- dlls/rpcrt4/rpc_transport.c | 73 ++++++----- 7 files changed, 259 insertions(+), 244 deletions(-)and it adds more casts as the previous patch as we need to cast down from CommonHdr to another Hdr. I tried to hide them thanks to rpc_castdown (ugly ;-) macro (could rename RpcPktCommonHdr into RpcPktHdr to safe a few changes, but still). There's still some uglyness in rpcrt4_http_prepare_out_pipe where getting rid of the union doesn't allow to predefine size of incoming packet... setting the type of pkt_from_server to HttpHdr hides the dust under the carpet, but IMO this should rather be dynamically allocated (like what's done in RPCRT4_default_receive_fragment) / (attached patch is wrong has it will write outside of local variable) so I'd rather keep in simple for now (no FAM, keep the union(s)!!)
Ok, let's stick with using the union.
-
Eric Pouech -
eric pouech (＠epo)
-
Huw Davies (＠huw)