From 2af0663b47e5d9740d609e8fb86c9ec527a29536 Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 08:21:24 -0700 Subject: [PATCH 1/9] Remove the unecessary lower-bound channel limit checks This commit removes the channelLimit < ENET_PROTOCOL_MINIMUM_CHANNEL_COUNT checks because they're always false (and aren't needed to ensure the channel limit is within-bounds). Here's why: 1. channelLimit is unsigned, so it's always zero or greater. 2. The 'if' branch already handles the case where channelLimit is zero, which is an alias for the maximum limit (per the API description). 3. Therefore, the else branch is only called when channelLimit is between 1 and ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT, inclusively. These bounds are perfectly valid, so the else condition is always false and isn't needed. --- include/enet.h | 5 ----- 1 file changed, 5 deletions(-) diff --git a/include/enet.h b/include/enet.h index 5263246..17839cc 100644 --- a/include/enet.h +++ b/include/enet.h @@ -4437,8 +4437,6 @@ extern "C" { if (!channelLimit || channelLimit > ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT) { channelLimit = ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT; - } else if (channelLimit < ENET_PROTOCOL_MINIMUM_CHANNEL_COUNT) { - channelLimit = ENET_PROTOCOL_MINIMUM_CHANNEL_COUNT; } host->randomSeed = (enet_uint32) (size_t) host; @@ -4690,10 +4688,7 @@ extern "C" { void enet_host_channel_limit(ENetHost *host, size_t channelLimit) { if (!channelLimit || channelLimit > ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT) { channelLimit = ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT; - } else if (channelLimit < ENET_PROTOCOL_MINIMUM_CHANNEL_COUNT) { - channelLimit = ENET_PROTOCOL_MINIMUM_CHANNEL_COUNT; } - host->channelLimit = channelLimit; } From f29dab052a7fc4fa3982ad1e8939637adbc95181 Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 08:37:22 -0700 Subject: [PATCH 2/9] Move the outgoing bandwidth fetch to where its checked The prior code fetches the /outgoing/ value but then follows it with a test on the /incoming/ value, which (to a new maintainers), might appear as a typo or copy-and-paste error. So we move the outgoing fetch to where the check is performed, similar to the incoming fetch-and-check pair prior to it. --- include/enet.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/enet.h b/include/enet.h index 17839cc..7a0b173 100644 --- a/include/enet.h +++ b/include/enet.h @@ -2217,18 +2217,17 @@ extern "C" { if (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER) { return -1; } - if (peer->incomingBandwidth != 0) { --host->bandwidthLimitedPeers; } peer->incomingBandwidth = ENET_NET_TO_HOST_32(command->bandwidthLimit.incomingBandwidth); - peer->outgoingBandwidth = ENET_NET_TO_HOST_32(command->bandwidthLimit.outgoingBandwidth); - if (peer->incomingBandwidth != 0) { ++host->bandwidthLimitedPeers; } + peer->outgoingBandwidth = ENET_NET_TO_HOST_32(command->bandwidthLimit.outgoingBandwidth); + if (peer->incomingBandwidth == 0 && host->outgoingBandwidth == 0) { peer->windowSize = ENET_PROTOCOL_MAXIMUM_WINDOW_SIZE; } else if (peer->incomingBandwidth == 0 || host->outgoingBandwidth == 0) { From 42df5349a11b4a0de6380e3e691c233be0c2a78a Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 09:02:27 -0700 Subject: [PATCH 3/9] Explicitly convert the socket type to an integer The incoming type is an enum, so explicitly convert it to before comparing against integers. --- include/enet.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/enet.h b/include/enet.h index 7a0b173..475f8c1 100644 --- a/include/enet.h +++ b/include/enet.h @@ -5447,7 +5447,7 @@ extern "C" { } ENetSocket enet_socket_create(ENetSocketType type) { - return socket(PF_INET6, type == ENET_SOCKET_TYPE_DATAGRAM ? SOCK_DGRAM : SOCK_STREAM, 0); + return socket(PF_INET6, (int)type == ENET_SOCKET_TYPE_DATAGRAM ? SOCK_DGRAM : SOCK_STREAM, 0); } int enet_socket_set_option(ENetSocket socket, ENetSocketOption option, int value) { From ac16add17174db0157182241d4c6a20d3bc12eff Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 09:05:08 -0700 Subject: [PATCH 4/9] Simplify the freeing of the result list Flagged by PVS Studio: V809. Verifying that a pointer value is not NULL is not required. The 'if (ptr != NULL)' check can be removed. The analyzer has detected a code fragment that can be simplified. The 'free()' function and 'delete' operator handle the null pointer correctly. So we can remove the pointer check. This allows us to delete an unnecessary string to make the code shorter and clearer. --- include/enet.h | 32 ++++++-------------------------- 1 file changed, 6 insertions(+), 26 deletions(-) diff --git a/include/enet.h b/include/enet.h index 475f8c1..a345007 100644 --- a/include/enet.h +++ b/include/enet.h @@ -5037,10 +5037,7 @@ extern "C" { } if (getaddrinfo(name, NULL, &hints, &resultList) != 0) { - if (resultList != NULL) { - freeaddrinfo(resultList); - } - + freeaddrinfo(resultList); return -1; } @@ -5049,29 +5046,18 @@ extern "C" { if (result->ai_family == AF_INET || (result->ai_family == AF_UNSPEC && result->ai_addrlen == sizeof(struct sockaddr_in))) { enet_inaddr_map4to6(((struct sockaddr_in*)result->ai_addr)->sin_addr, &out->host); out->sin6_scope_id = 0; - - if (resultList != NULL) { - freeaddrinfo(resultList); - } - + freeaddrinfo(resultList); return 0; + } else if (result->ai_family == AF_INET6 || (result->ai_family == AF_UNSPEC && result->ai_addrlen == sizeof(struct sockaddr_in6))) { memcpy(&out->host, &((struct sockaddr_in6*)result->ai_addr)->sin6_addr, sizeof(struct in6_addr)); out->sin6_scope_id = (enet_uint16) ((struct sockaddr_in6*)result->ai_addr)->sin6_scope_id; - - if (resultList != NULL) { - freeaddrinfo(resultList); - } - + freeaddrinfo(resultList); return 0; } } } - - if (resultList != NULL) { - freeaddrinfo(resultList); - } - + freeaddrinfo(resultList); return -1; } @@ -5356,7 +5342,6 @@ extern "C" { ((uint32_t *)&address->host.s6_addr)[3] = sin->sin_addr.s_addr; freeaddrinfo(resultList); - return 0; } else if(result->ai_family == AF_INET6) { @@ -5366,16 +5351,11 @@ extern "C" { address->sin6_scope_id = sin->sin6_scope_id; freeaddrinfo(resultList); - return 0; } } } - - - if (resultList != NULL) { - freeaddrinfo(resultList); - } + freeaddrinfo(resultList); return enet_address_set_host_ip(address, name); } /* enet_address_set_host_old */ From 608b68e2fa8892d9df38b64e7cfe9b46f68831b8 Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 09:15:19 -0700 Subject: [PATCH 5/9] Explicitly wrap the host pt into the random seed --- include/enet.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/enet.h b/include/enet.h index a345007..fc29a07 100644 --- a/include/enet.h +++ b/include/enet.h @@ -4438,7 +4438,7 @@ extern "C" { channelLimit = ENET_PROTOCOL_MAXIMUM_CHANNEL_COUNT; } - host->randomSeed = (enet_uint32) (size_t) host; + host->randomSeed = (enet_uint32) ((uintptr_t) host % UINT32_MAX); host->randomSeed += enet_host_random_seed(); host->randomSeed = (host->randomSeed << 16) | (host->randomSeed >> 16); host->channelLimit = channelLimit; From d3fa74fa1f80e0014c94c9774f6ae4d93d647e0a Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 09:19:17 -0700 Subject: [PATCH 6/9] Eliminate an always-true pointer check 'packet' is already guaranteed to be not NULL because prior in the function we check: if (packet == NULL) { goto notifyError; }. So we replace these checks with an assertion, because it's what we expect given the current state of the code. --- include/enet.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/include/enet.h b/include/enet.h index fc29a07..6b17c6a 100644 --- a/include/enet.h +++ b/include/enet.h @@ -35,6 +35,7 @@ #ifndef ENET_INCLUDE_H #define ENET_INCLUDE_H +#include #include #include #include @@ -4328,10 +4329,9 @@ extern "C" { memset(incomingCommand->fragments, 0, (fragmentCount + 31) / 32 * sizeof(enet_uint32)); } - if (packet != NULL) { - ++packet->referenceCount; - peer->totalWaitingData += packet->dataLength; - } + assert(packet != NULL); + ++packet->referenceCount; + peer->totalWaitingData += packet->dataLength; enet_list_insert(enet_list_next(currentCommand), incomingCommand); @@ -4353,7 +4353,8 @@ extern "C" { goto notifyError; } - if (packet != NULL && packet->referenceCount == 0) { + assert(packet != NULL); + if (packet->referenceCount == 0) { callbacks.packet_destroy(packet); } From 82757d0cc71e23e2f9a67dba15ce05c30eae2b8e Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 09:39:28 -0700 Subject: [PATCH 7/9] Clarify that that the first buffer member is used --- include/enet.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/include/enet.h b/include/enet.h index 6b17c6a..d555f17 100644 --- a/include/enet.h +++ b/include/enet.h @@ -3133,12 +3133,12 @@ extern "C" { currentPeer->packetsLost = 0; } - host->buffers->data = headerData; + host->buffers[0].data = headerData; if (host->headerFlags & ENET_PROTOCOL_HEADER_FLAG_SENT_TIME) { header->sentTime = ENET_HOST_TO_NET_16(host->serviceTime & 0xFFFF); - host->buffers->dataLength = sizeof(ENetProtocolHeader); + host->buffers[0].dataLength = sizeof(ENetProtocolHeader); } else { - host->buffers->dataLength = (size_t) &((ENetProtocolHeader *) 0)->sentTime; + host->buffers[0].dataLength = (size_t) &((ENetProtocolHeader *) 0)->sentTime; } shouldCompress = 0; @@ -3159,9 +3159,9 @@ extern "C" { } header->peerID = ENET_HOST_TO_NET_16(currentPeer->outgoingPeerID | host->headerFlags); if (host->checksum != NULL) { - enet_uint32 *checksum = (enet_uint32 *) &headerData[host->buffers->dataLength]; + enet_uint32 *checksum = (enet_uint32 *) &headerData[host->buffers[0].dataLength]; *checksum = currentPeer->outgoingPeerID < ENET_PROTOCOL_MAXIMUM_PEER_ID ? currentPeer->connectID : 0; - host->buffers->dataLength += sizeof(enet_uint32); + host->buffers[0].dataLength += sizeof(enet_uint32); *checksum = host->checksum(host->buffers, host->bufferCount); } From 10be52fbd113da04864066c69d8c01539ff3c9e7 Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 09:39:53 -0700 Subject: [PATCH 8/9] Avoid pointing to invalid data on scope closure --- include/enet.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/enet.h b/include/enet.h index d555f17..ca051fd 100644 --- a/include/enet.h +++ b/include/enet.h @@ -3176,6 +3176,9 @@ extern "C" { enet_protocol_remove_sent_unreliable_commands(currentPeer); if (sentLength < 0) { + // The local 'headerData' array (to which 'data' is assigned) goes out + // of scope on return from this function, so ensure we no longer point to it. + host->buffers[0].data = NULL; return -1; } @@ -3184,6 +3187,10 @@ extern "C" { host->totalSentPackets++; } + // The local 'headerData' array (to which 'data' is assigned) goes out + // of scope on return from this function, so ensure we no longer point to it. + host->buffers[0].data = NULL; + return 0; } /* enet_protocol_send_outgoing_commands */ From a1a909b4ee86f9c7bab0cab2668ff40bbf33c217 Mon Sep 17 00:00:00 2001 From: kcgen Date: Thu, 28 Jul 2022 09:51:11 -0700 Subject: [PATCH 9/9] Clarify the peer pointer's validity using an assertion (CWE-476) The prior switch statement dereferences the peer pointer in each valid case, so if peer really was NULL then the program will have crashed due to derefercing a null pointer. Thus, this peer != NULL statement is misleading: it implies that peer might legitimately be NULL, when infact, we expect peer to be valid at this point. So instead, we replace this checking with our expectation (using an assertion). Flagged by Coverity: In enet_protocol_handle_incoming_commands(_ENetHost *, _ENetEvent *): All paths that lead to this null pointer comparison already dereference the pointer earlier (CWE-476) --- include/enet.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/enet.h b/include/enet.h index ca051fd..becacaf 100644 --- a/include/enet.h +++ b/include/enet.h @@ -2641,7 +2641,8 @@ extern "C" { goto commandError; } - if (peer != NULL && (command->header.command & ENET_PROTOCOL_COMMAND_FLAG_ACKNOWLEDGE) != 0) { + assert(peer); + if ((command->header.command & ENET_PROTOCOL_COMMAND_FLAG_ACKNOWLEDGE) != 0) { enet_uint16 sentTime; if (!(flags & ENET_PROTOCOL_HEADER_FLAG_SENT_TIME)) {