Network: Unify backwards compatibility checks #16800
Conversation
SmallJoker
added
the
Maintenance
| *pkt >> id >> type; | ||
| m_env.addActiveObject(id, type, pkt->readLongString()); | ||
| } | ||
| } catch (PacketError &e) { |
There was a problem hiding this comment.
This should never occur. This change will cause the client to disconnect and show a technical error message (get_packet_dbg_info). If this change of behaviour is undesired, I can undo that on request.
| bool ephemeral = false; | ||
|
|
||
| *pkt >> server_id >> spec.name >> spec.gain >> (u8 &)type >> pos >> object_id >> spec.loop; | ||
| *pkt >> spec.fade >> spec.pitch; |
There was a problem hiding this comment.
Note to reviewers: Added in 5.0.0-dev, which is guaranteed to be available.
| try { | ||
| *pkt >> world_pos; | ||
| *pkt >> size; | ||
| *pkt >> world_pos >> size; |
There was a problem hiding this comment.
Note to reviewers: Added in 0.4.16 or so, which is guaranteed to be available.
| errorstream << "Server::ProcessData(): SendFailedException: " | ||
| << "what=" << e.what() | ||
| << std::endl; | ||
| } catch (PacketError &e) { |
There was a problem hiding this comment.
Note to reviewers: Moved to Server::Receive, where almost all other exceptions are handled.
SmallJoker
added
the
Waiting (on dependency)
| >> height >> thickness >> speed; | ||
|
|
||
| if (pkt->getRemainingBytes() >= 4) { | ||
| if (pkt->getRemainingBytes()) { |
There was a problem hiding this comment.
I would prefer >= 0 to make the intent clearer
There was a problem hiding this comment.
Someone might think that >= 0 should be changed to >= 4 to be sure there's enough bytes to read. That's however incorrect. It should always be checked against 0 to catch incorrectly formatted packets.
How about != 0 instead?
There was a problem hiding this comment.
or: introduce a method and make it pkt->hasRemainingBytes(), given how frequent this pattern will be?
similar to how containers have empty().
src/server.cpp
Outdated
| << e.what() << std::endl; | ||
| } catch (const SerializationError &e) { | ||
| } catch (SerializationError &e) { | ||
| e.appendBacktrace(get_packet_dbg_info(pkt, true)); |
There was a problem hiding this comment.
strictly speaking there's no guarantee that the SerializationError actually directly related to an offset in the packet (e.g. with wrapped SAO cmds)
There was a problem hiding this comment.
Not necessarily, but this is better than ... well.. no information at all.
This commit replaces all uses of PacketError with 'getRemainingBytes' to
more reliably catch issues related to improper read/write implementations.
Furthermore, conditions like 'getRemainingBytes() >= 4' could hide potential
issues, or even cause misbehaviour if used multiple times. Hence,
'do {} while(0);' + 'break' is now used to avoid such edge-cases.
PacketError handling is moved consistently to
- Server::Receive
- Client::ReceiveAll (only to append details, rethrown.)
SmallJoker
force-pushed
the
pr_16800_PacketError_cleanup
branch
from
9a6c02d to
1dbf4f1
Compare
SmallJoker
removed
the
Waiting (on dependency)
3 participants
This commit replaces all uses of PacketError with 'getRemainingBytes' to more reliably catch issues related to improper read/write implementations. Furthermore, conditions like 'getRemainingBytes() >= 4' could hide potential issues, or even cause misbehaviour if used multiple times. Hence, 'do {} while(0);' + 'break' is now used to avoid such edge-cases.
PacketError handling is moved consistently to
This is a follow-up on #16796.
To do
This PR is Ready for Review.
How to test