Skip to content

Fix exception on Bad argument#3925

Open
lLukDreams wants to merge 1 commit into
citizenfx:masterfrom
lLukDreams:fix/mono
Open

Fix exception on Bad argument#3925
lLukDreams wants to merge 1 commit into
citizenfx:masterfrom
lLukDreams:fix/mono

Conversation

@lLukDreams
Copy link
Copy Markdown

Goal of this PR

Improve the robustness and security of event argument handling by preventing malformed or malicious arguments from disrupting server functionality.


How is this PR achieving the goal

This PR strengthens argument conversion and error handling within the event handler system:

  • Enhances the ChangeType method to safely handle InvalidCastException, FormatException, and OverflowException, preventing crashes or unintended behavior caused by invalid input.
  • Adds detailed debug logging when argument conversion fails, including event name, source, and involved types, making issues easier to trace and diagnose.
  • Updates GetPassArguments to optionally accept an eventName, allowing error messages to include contextual information for better debugging.
  • Mitigates an exploit where malicious clients could send incorrect argument types to break or disable critical server events.

This PR applies to the following area(s)

  • Server
  • ScRT: C#

Successfully tested on

Game builds: Latest

Platforms: Windows


Checklist

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

@github-actions github-actions Bot added the invalid Requires changes before it's considered valid and can be (re)triaged label Apr 13, 2026
@forzayt
Copy link
Copy Markdown

forzayt commented Apr 21, 2026

Good fix 👍 Maybe worth adding more context in logs (arg index + expected vs actual type), cause defaulting values may hide invalid event payloads

meme

@lLukDreams lLukDreams changed the title Fix Crash on Bad argument Fix exception on Bad argument Apr 28, 2026
Copy link
Copy Markdown
Contributor

@FabianTerhorst FabianTerhorst left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution 👍

@FabianTerhorst FabianTerhorst added ready-to-merge This PR is enqueued for merging and removed invalid Requires changes before it's considered valid and can be (re)triaged labels Apr 28, 2026
@FabianTerhorst
Copy link
Copy Markdown
Contributor

Good fix 👍 Maybe worth adding more context in logs (arg index + expected vs actual type), cause defaulting values may hide invalid event payloads

meme meme

Feel free to do a follow up once this is merged.

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

Labels

ready-to-merge This PR is enqueued for merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants