Possible attempts to read outside array size in OPUNetTransportLayer

[Brett208]

I am getting the warning below in many places across the codebase where peerInfo is accessed. I suspect it has to to do with using the line playerNetID & 7 on all occasions.

Warning C6385 Reading invalid data from 'this->peerInfo': the readable size is '168' bytes, but '224' bytes may be read. NetFixClient \NETFIXCLIENT\CLIENT\OPUNETTRANSPORTLAYER.CPP 509

As a side note, it would be nice to explain why we have to apply `& 7' to the playerNetID before accessing the peerInfo array.

https://docs.microsoft.com/en-us/cpp/code-quality/c6385?view=vs-2019

[DanRStevens]

The playerNetID is basically a struct with 2 bitfields. The lower 3 bits (mask of 7), is the playerIndex. The upper bits I believe were a time stamp. I'm not sure it served any real purpose. The playerIndex should be unique within a game, and there is no guarantee the timestamp would make things unique in a more global sense.

We should change the type for playerNetID to use a struct, rather than an int:

struct PlayerNetID {
  std::size_t playerIndex:3;
  std::time_t timeStamp:29; // Or whatever the correct type and size is
};

I suspect the two types will need to match for them to share bits in the same byte.

---

As for the error, it looks like the array was declared to have 6 elements for 6 players (0..5), but the bit mask for 3 bits allows for 8 values (0..7).

I suppose we could increase the array size, or perhaps extract the playerIndex calculation into a method and error check it.

Probably easiest to just increase the array size. It's not like it amounts to a whole lot of extra memory.

[Brett208]

Hmm, I would be okay with either proposed solution. Neither is particular elegant though. If we increase the array size, I would be tempted to start error checking to insure it was never set above the 6th index, so I would probably lean toward using a function to error check playerIndex.

Thanks for diagnosing. I like the idea of using the bitfield struct proposed for PlayerNetID.

[Brett208]

I tried playing with the struct PlayerNetID idea.

I came up with this:

struct PlayerNetID
{
	PlayerNetID(int value) : value(value)
	{
		if (GetPlayerIndex() >= 6) {
			throw std::runtime_error("Maximum player index is 5");
		}
	}

	inline void Clear() { value = 0; }
	inline bool IsEmpty() const { return value == 0; }

	inline std::size_t GetPlayerIndex() const { return value & 7; }
	inline std::time_t GetTime() const { return value & ~7; }
	inline int GetRawValue() const { return value; }

private:
	int value;
};

I don't know. Seems maybe off track.

[DanRStevens]

If you use bitfields, you can eliminate the & 7 parts.

---

I probably wouldn't error check in the constructor. The values comes in from over the network, so there's really no guarantee on what they will be. I don't want to burden code deep within the network layer with potential exceptions during value construction. Instead, I would check and possibly throw at the point where the value is read.

Actually, the network structs are effectively overlaid on top of existing memory, so there isn't any construction. The data just is. For that reason, I would recommend sticking to a struct with all public fields, and no constructor. Having an error checking accessor is fine. You'd want to avoid features that would make the type non-POD though. Well, I guess POD is the old name for it, can't quite remember what the concept was split into or which one of them we actually want. Basically we want to be able to cast arbitrary memory to our type, and be able to work with it.

[Brett208]

I can't figure out how to efficiently set the int representation into the proposed bitfield.

[DanRStevens]

I'm afraid I don't have much context on that. For reading from the network, the bytes are reinterpret_cast-ed to structs with the given type, and then read from.

For assignment from an int, I guess you could use a union for that:

struct PlayerNetID {
    union {
        struct {
            std::size_t playerIndex:3;
            std::size_t timeStamp:29;
        };
        int intValue;
    };
};

Or you can set the fields individually:

PlayerNetId playerNetId;
playerNetId.playerIndex = 0;
playerNetId.timeStamp = timeGetTime();

[Brett208]

Setting intValue and then accessing the struct would be considered type punning right? I'm guessing since you are posting the solution you are okay with that? Seems to be mixed opinions about it on the internet.

[DanRStevens]

I would prefer not doing it, but it may be expedient.

You can also break apart an int using the bit masking techniques, and assign the results to individual fields. That might perhaps be better.

playerNetId.payerIndex = value & 7;
playerNetId.timeStamp = (value & ~7) >> 3;