Skip to content

Commit 22865ff

Browse files
committed
fix use-after-free on next_sub in broker fan-out loops
Both subscription fan-out loops snapshot next_sub before a write that can drive a re-entrant WebSocket close. That close runs BrokerSubs_RemoveClient and frees the subscriber's sub nodes, while pending_remove only defers the client struct, so next_sub itself can be freed. Revalidate it against broker->subs after the write, like MqttBroker_Step does for the client list.
1 parent 913dedb commit 22865ff

1 file changed

Lines changed: 33 additions & 0 deletions

File tree

src/mqtt_broker.c

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2959,6 +2959,29 @@ static void BrokerSubs_Remove(MqttBroker* broker, BrokerClient* bc,
29592959
#endif
29602960
}
29612961

2962+
#ifndef WOLFMQTT_STATIC_MEMORY
2963+
/* Confirm a fan-out loop's snapshotted successor is still linked in
2964+
* broker->subs. A peer-initiated WS LWS_CALLBACK_CLOSED delivered during a
2965+
* fan-out write runs BrokerSubs_RemoveClient, which frees every BrokerSub
2966+
* owned by the closing subscriber - and that can include the node a loop saved
2967+
* as next_sub. The pending_remove deferral keeps the BrokerClient struct alive
2968+
* across the spin, but the sub nodes are freed regardless, so snapshotting
2969+
* next_sub guards only against sub->next moving, not against next_sub itself
2970+
* being freed. Re-check it before the next dereference, mirroring the
2971+
* client-list revalidation in MqttBroker_Step. */
2972+
static int BrokerSubs_StillLinked(MqttBroker* broker, const BrokerSub* node)
2973+
{
2974+
BrokerSub* cur = broker->subs;
2975+
while (cur != NULL) {
2976+
if (cur == node) {
2977+
return 1;
2978+
}
2979+
cur = cur->next;
2980+
}
2981+
return 0;
2982+
}
2983+
#endif
2984+
29622985
/* -------------------------------------------------------------------------- */
29632986
/* Packet ID generation */
29642987
/* -------------------------------------------------------------------------- */
@@ -4067,6 +4090,11 @@ static void BrokerClient_PublishWillImmediate(MqttBroker* broker,
40674090
}
40684091
}
40694092
#ifndef WOLFMQTT_STATIC_MEMORY
4093+
/* The write above can drive a re-entrant WS close that frees next_sub;
4094+
* stop the walk if it is gone. */
4095+
if (next_sub != NULL && !BrokerSubs_StillLinked(broker, next_sub)) {
4096+
break;
4097+
}
40704098
sub = next_sub;
40714099
#endif
40724100
}
@@ -5476,6 +5504,11 @@ static int BrokerHandle_Publish(BrokerClient* bc, int rx_len,
54765504
}
54775505
}
54785506
}
5507+
/* The fan-out write above can drive a re-entrant WS close that
5508+
* frees next_sub; stop the walk if it is gone. */
5509+
if (next_sub != NULL && !BrokerSubs_StillLinked(broker, next_sub)) {
5510+
break;
5511+
}
54795512
sub = next_sub;
54805513
#endif
54815514
}

0 commit comments

Comments
 (0)