From 1eba6829f749baa8dddc83f7d40b66ef3fcb8c76 Mon Sep 17 00:00:00 2001 From: zhoujiaqi Date: Fri, 21 Jul 2023 14:23:59 +0800 Subject: [PATCH] Fix interconnection udpifc UAF in ipc teardown Problem details: In function chunkTransportStateEntryInitialized got **wrong** valid with motNodeID When tear down happen in udpifc, the outgoing route may not delete entry in shared htab. The htab will be shared with rxthread. After main thread tear down, some of conns will be freed by MemoryContextReset But in the same time, htab will hold the invalid conn ptr in rxthread Then got core dump. Fixed: Changed the chunkTransportStateEntryInitialized, using the getChunkTransportState* to get the right entry Also do not direct access states in ChunkTransportState. --- contrib/interconnect/ic_internal.h | 10 +++++++++- contrib/interconnect/udp/ic_udpifc.c | 7 +++++-- src/include/cdb/cdbinterconnect.h | 11 ++++++++++- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/contrib/interconnect/ic_internal.h b/contrib/interconnect/ic_internal.h index c09d89e9697..2ce6075d7c4 100644 --- a/contrib/interconnect/ic_internal.h +++ b/contrib/interconnect/ic_internal.h @@ -165,7 +165,15 @@ typedef struct ChunkTransportStateEntry int motNodeId; bool valid; - /* Connection array */ + /* Connection array + * + * MUST pay attention: use getMotionConn to get MotionConn. + * must not use `->conns[index]` to get MotionConn. Because the struct + * MotionConn is a base structure for MotionConnTCP and + * MotionConnUDP. After connection setup, the `conns` will be fill + * with MotionConnUDP/MotionConnTCP, but the pointer still is + * MotionConn which should use `CONTAINER_OF` to get the real object. + */ MotionConn *conns; int numConns; diff --git a/contrib/interconnect/udp/ic_udpifc.c b/contrib/interconnect/udp/ic_udpifc.c index a5cf7b7aaaa..b125bda62f5 100644 --- a/contrib/interconnect/udp/ic_udpifc.c +++ b/contrib/interconnect/udp/ic_udpifc.c @@ -3413,10 +3413,13 @@ static bool chunkTransportStateEntryInitialized(ChunkTransportState *transportStates, int16 motNodeID) { - if (motNodeID > transportStates->size || !transportStates->states[motNodeID - 1].valid) + ChunkTransportStateEntry *pEntry = NULL; + if (motNodeID > transportStates->size) { return false; + } - return true; + getChunkTransportStateNoValid(transportStates, motNodeID, &pEntry); + return pEntry->valid; } /* diff --git a/src/include/cdb/cdbinterconnect.h b/src/include/cdb/cdbinterconnect.h index 4d7e733448e..f52b9e0bc4c 100644 --- a/src/include/cdb/cdbinterconnect.h +++ b/src/include/cdb/cdbinterconnect.h @@ -192,7 +192,16 @@ typedef struct MotionConnSentRecordTypmodEnt typedef struct ChunkTransportState { - /* array of per-motion-node chunk transport state */ + /* array of per-motion-node chunk transport state + * + * MUST pay attention: use getChunkTransportStateNoValid/getChunkTransportState + * to get ChunkTransportStateEntry. + * must not use `->states[index]` to get ChunkTransportStateEntry. Because the struct + * ChunkTransportStateEntry is a base structure for ChunkTransportStateEntryTCP and + * ChunkTransportStateEntryUDP. After interconnect setup, the `states` will be fill + * with EntryUDP/EntryTCP, but the pointer still is ChunkTransportStateEntry which + * should use `CONTAINER_OF` to get the real object. + */ int size; struct ChunkTransportStateEntry *states;