BUG: enet_peer_disconnect and enet_peer_disconnect_now accessing deallocated peer->channels (NULL pointer access)

[mman]

If you look at the logic in enet_peer_disconnect and enet_peer_disconnect_now, they both first call enet_peer_reset_queues which will deallocate peer->channels and set it to NULL. Both methods will then try to enet_peer_queue_outgoing_command with a newly created ENET_PROTOCOL_COMMAND_DISCONNECT.

This will in turn invoke enet_peer_setup_outgoing_command that will attempt to read peer->channels that is set to NULL. In case of disconnect, the peer->channels variable is actually just read, and never written to but it generates a runtime warning.

Suggested fix should probably be initializing the local channel variable only when peer->channels is not NULL. PR to follow.

Freeing of peer->channels in enet_peer_reset_queues

https://github.com/lsalzman/enet/blob/74cea7abf52ddd355146aeb0a4077d2b95368122/peer.c#L341

Logic of enet_peer_disconnect_now

https://github.com/lsalzman/enet/blob/74cea7abf52ddd355146aeb0a4077d2b95368122/peer.c#L510

https://github.com/lsalzman/enet/blob/74cea7abf52ddd355146aeb0a4077d2b95368122/peer.c#L516

Logic of enet_peer_disconnect

https://github.com/lsalzman/enet/blob/74cea7abf52ddd355146aeb0a4077d2b95368122/peer.c#L541

https://github.com/lsalzman/enet/blob/74cea7abf52ddd355146aeb0a4077d2b95368122/peer.c#L552

Logic of enet_peer_queue_outgoing_command, calling enet_peer_setup_outgoing_command

https://github.com/lsalzman/enet/blob/74cea7abf52ddd355146aeb0a4077d2b95368122/peer.c#L696

enet_peer_setup_outgoing_command accessing peer->channels that is set to NULL

https://github.com/lsalzman/enet/blob/74cea7abf52ddd355146aeb0a4077d2b95368122/peer.c#L622

Looking at the code, both disconnect and disconnect_now are supposed to prepare a DISCONNECT command, and send it over to network.

[mman]

The following fix will make sure that analyzer is happy and not producing warning on dereferencing NULL memory, and it will also make sure that the potentially wrong usage of channel variable will crash rather then using random memory.

diff --git a/peer.c b/peer.c
index 83c57e7..573cab1 100644
--- a/peer.c
+++ b/peer.c
@@ -621,7 +621,10 @@ enet_peer_queue_acknowledgement (ENetPeer * peer, const ENetProtocol * command,
 void
 enet_peer_setup_outgoing_command (ENetPeer * peer, ENetOutgoingCommand * outgoingCommand)
 {
-    ENetChannel * channel = & peer -> channels [outgoingCommand -> command.header.channelID];
+    ENetChannel * channel = NULL;
+    if (peer -> channels != NULL) {
+        channel = & peer -> channels [outgoingCommand -> command.header.channelID];
+    }

     peer -> outgoingDataTotal += enet_protocol_command_size (outgoingCommand -> command.header.command) + outgoingCommand -> fragmentLength;

[lsalzman]

It's not derefing channels at all. It's just taking the address, hence the &. In this case the channel ID of the disconnect message is 0xFF, so it skips ever actually reading from it. While this may be confusing, it's not a bug.

[lsalzman]

Fixed.