Cleanup static analysis issues

[kcgen]

Fixes https://github.com/zpl-c/enet/issues/35

Suggest reviewing commit-by-commit.

[kcgen]

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]

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.

[inlife]

@kcgen I had to revert this specific commit: https://github.com/zpl-c/enet/commit/d3fa74fa1f80e0014c94c9774f6ae4d93d647e0a

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 a1a909b4ee86f9c7bab0cab2668ff40bbf33c217, since it was dependent on the previous.

[kcgen]

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]

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

[kcgen]

EDIT: also had to revert commit https://github.com/zpl-c/enet/commit/a1a909b4ee86f9c7bab0cab2668ff40bbf33c217, 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]

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

[kcgen]

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.