-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: specify log exception. #3089
Changes from 7 commits
e95220f
4cba9dc
63bed84
f056456
8d5d00c
f01d3a7
9a4d12d
fe8edd0
92a8116
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -327,9 +327,16 @@ protected internal BigInteger GetRandom() | |
/// <param name="state">The message of the log.</param> | ||
protected internal void RuntimeLog(byte[] state) | ||
{ | ||
if (state.Length > MaxNotificationSize) throw new ArgumentException(null, nameof(state)); | ||
string message = Utility.StrictUTF8.GetString(state); | ||
Log?.Invoke(this, new LogEventArgs(ScriptContainer, CurrentScriptHash, message)); | ||
if (state.Length > MaxNotificationSize) throw new ArgumentException("Message is too long.", nameof(state)); | ||
try | ||
{ | ||
string message = Utility.StrictUTF8.GetString(state); | ||
Log?.Invoke(this, new LogEventArgs(ScriptContainer, CurrentScriptHash, message)); | ||
} | ||
catch | ||
{ | ||
throw new ArgumentException("Failed to convert byte array to string: Invalid or non-printable UTF-8 sequence detected.", nameof(state)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add the failed data in hex? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure, cause user passes in bytes, if you show them hex, does that help at all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the message, both options are good for me |
||
} | ||
} | ||
|
||
/// <summary> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this change fork something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will still fault. Originally it fault, now it still fault, but fault with different exception, a more reasonable exception message.