Skip to content

Conversation

@asamy
Copy link
Contributor

@asamy asamy commented Oct 24, 2025

Changes Proposed

It is possible that local::conv::to_utf/from_utf8 throw an exception if an invalid char is provided, as far as I am aware a network message can contain user[?] provided messages and thus can crash the server.

It is probably a good idea to use std::expected or std::optional for these error cases in such paths for better error handling, but this is for a different PR.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds exception handling for potential character encoding conversion errors in network message processing. The changes prevent server crashes when invalid characters are provided in network messages by catching boost::locale::conv::conversion_error exceptions.

Key Changes:

  • Added try-catch blocks around boost::locale::conv::to_utf and from_utf calls to handle conversion errors gracefully
  • Functions now return empty values instead of crashing when encountering invalid character encodings

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@asamy
Copy link
Contributor Author

asamy commented Oct 25, 2025

To highlight the addString() issue when there is an error that is unbeknownst to the caller:

I recommend having all add functions return std::optional (or std::expected or similar) and using .and_then() on each return so that the caller doesn't keep adding when there is no space or there was an error, and after all is added properly, it can be sent, so something like this:

msg.add<uint16_t>(0)
   .and_then(addString)
   .and_then(send)
   .or_else(error)

and_then() will only be called if the previous call returned a value (no error), so that it will continue adding when safe, but stop when not, and or_else() will return the value if it has one, or the return of error() if not.

If this sounds good, then create an issue out of this comment and improve on it, as this is better suited for a different PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants