Cleanup static analysis issues #37

kcgen · 2022-07-28T17:04:11Z

Fixes #35

Suggest reviewing commit-by-commit.

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.

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.

The incoming type is an enum, so explicitly convert it to before comparing against integers.

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.

'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.

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)

kcgen · 2022-07-28T17:22:57Z

Is the appveyor CI job broken?

Although it reports "build failed" above, the build itself appears to have passed, and it's the test executable that isn't being found:

zpl-zak · 2022-07-28T19:02:58Z

Is the appveyor CI job broken?

Although it reports "build failed" above, the build itself appears to have passed, and it's the test executable that isn't being found:

It indeed seems to be broken, we'll have a look into that!

Thanks for the contribution. It's much appreciated! We'll review the changes in the following days.

zpl-zak

Thank you for your contribution!

For those wondering why this commit's author isn't set to the Enet maintainer(s), in this case, all changes made in this commit were authored by myself (ref: zpl-c/enet#37)

inlife · 2022-07-30T18:10:17Z

@kcgen I had to revert this specific commit: d3fa74f

There were issues with crashes on Win related to one of the asserts, the null pointer check was actually used actively. So yeah. :)

EDIT: also had to revert commit a1a909b, since it was dependent on the previous.

kcgen · 2022-07-30T18:19:30Z

Good catch @inlife -- I didn't see the discardCommand: label:

Based on this, I believe the first is still valid because there are no external goto's that can change the state of that sequence.

But the second check can be "jumped into", so all bets are off there.

Do you want to try putting the first back in, and retry the test?

inlife · 2022-07-30T18:24:10Z

Ok, that will most likely work fine, gonna do that.

kcgen · 2022-07-30T18:46:01Z

EDIT: also had to revert commit a1a909b, since it was dependent on the previous.

If we look at the switch statement preceed the assert(peer); and think about peer possibly being NULL on entry:

All of these functions test peer's validity by dereferencing the pointer straight away:

enet_protocol_handle_acknowledge():

if (peer->state == ENET_PEER_STATE_DISCONNECTED)

enet_protocol_handle_verify_connect():

if (peer->state != ENET_PEER_STATE_CONNECTING)

enet_protocol_handle_disconnect():

if (peer->state == ENET_PEER_STATE_DISCONNECTED || peer->state == ENET_PEER_STATE_ZOMBIE || peer->state == ENET_PEER_STATE_ACKNOWLEDGING_DISCONNECT)

enet_protocol_handle_ping():

if (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER)

enet_protocol_handle_send_reliable():

if (command->header.channelID >= peer->channelCount || (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER))

enet_protocol_handle_send_unsequenced():

if (command->header.channelID >= peer->channelCount || (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER))

enet_protocol_handle_send_unreliable():

if (command->header.channelID >= peer->channelCount ||
          (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER))

enet_protocol_handle_send_fragment():

if (command->header.channelID >= peer->channelCount || (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER))

enet_protocol_handle_bandwidth_limit():

if (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER)

enet_protocol_handle_send_unreliable_fragment():

if (command->header.channelID >= peer->channelCount || (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER))

enet_protocol_handle_throttle_configure():

if (peer->state != ENET_PEER_STATE_CONNECTED && peer->state != ENET_PEER_STATE_DISCONNECT_LATER)

enet_protocol_handle_disconnect():

if (peer->state == ENET_PEER_STATE_DISCONNECTED || peer->state == ENET_PEER_STATE_ZOMBIE ||
            peer->state == ENET_PEER_STATE_ACKNOWLEDGING_DISCONNECT
        )

The only switch cases that don't dereference peer are COMMAND_CONNECT, which jumps out if peer is NULL, and default, which also jumps out.

So if we believe the assert(peer) is bad -- then we need to fix all of the above functions and check for NULL there too.

inlife · 2022-07-31T14:14:11Z

Would you like to create another PR that would reflect the suggested changes?

kcgen · 2022-07-31T15:07:39Z

I can help - but I'm not sure what we should do.

It looks like the enet_protocol_ functions expect and rely on peer being valid.

That is, "normal operation" doesn't involve passing NULL peer values into them. With this expectation, I would enforce this assumed state with an assert(peer); in each of these functions (before they try using the peer pointer).

Then in this function with the big switch statement, we'd bail out prior to the switch if peer is null, except for the connect case.

kcgen added 9 commits July 28, 2022 09:41

Explicitly convert the socket type to an integer

42df534

The incoming type is an enum, so explicitly convert it to before comparing against integers.

Explicitly wrap the host pt into the random seed

608b68e

Eliminate an always-true pointer check

d3fa74f

'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.

Clarify that that the first buffer member is used

82757d0

Avoid pointing to invalid data on scope closure

10be52f

zpl-zak approved these changes Jul 29, 2022

View reviewed changes

inlife merged commit fcbc330 into zpl-c:master Jul 29, 2022

kcgen deleted the dosbox-staging/cleanup-1 branch July 30, 2022 00:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup static analysis issues #37

Cleanup static analysis issues #37

kcgen commented Jul 28, 2022

kcgen commented Jul 28, 2022

zpl-zak commented Jul 28, 2022

zpl-zak left a comment

inlife commented Jul 30, 2022 •

edited

Loading

kcgen commented Jul 30, 2022

inlife commented Jul 30, 2022

kcgen commented Jul 30, 2022

inlife commented Jul 31, 2022

kcgen commented Jul 31, 2022

Cleanup static analysis issues #37

Cleanup static analysis issues #37

Conversation

kcgen commented Jul 28, 2022

kcgen commented Jul 28, 2022

zpl-zak commented Jul 28, 2022

zpl-zak left a comment

Choose a reason for hiding this comment

inlife commented Jul 30, 2022 • edited Loading

kcgen commented Jul 30, 2022

inlife commented Jul 30, 2022

kcgen commented Jul 30, 2022

inlife commented Jul 31, 2022

kcgen commented Jul 31, 2022

inlife commented Jul 30, 2022 •

edited

Loading