-
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
[Neo Plugin Bug]fix 3296 #3299
[Neo Plugin Bug]fix 3296 #3299
Changes from 4 commits
a25c723
c183732
1378d24
dc0b574
612497a
512d738
ae94667
e04e1be
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 |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
using Neo.SmartContract; | ||
using Neo.VM; | ||
using Neo.VM.Types; | ||
using System.Text; | ||
|
||
namespace Neo.Plugins.ApplicationLogs.Store | ||
{ | ||
|
@@ -159,9 +160,13 @@ public Guid PutStackItemState(StackItem stackItem) | |
{ | ||
_snapshot.Put(key, BinarySerializer.Serialize(stackItem, ExecutionEngineLimits.Default with { MaxItemSize = (uint)Settings.Default.MaxStackSize })); | ||
} | ||
catch (NotSupportedException) | ||
catch (Exception ex) | ||
{ | ||
_snapshot.Put(key, BinarySerializer.Serialize(StackItem.Null, ExecutionEngineLimits.Default with { MaxItemSize = (uint)Settings.Default.MaxStackSize })); | ||
var detailedException = ex.Message; | ||
if (detailedException.Length > 1024) | ||
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. Catching any exception is enough to solve the original problem. Why should we change the behavior and serialize exception message instead of 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. Null or exception both works. Using 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. We think that it's not quite correct to replace the original stackitem by either Please, take a look at the way how this case is handled in NeoGo: https://github.com/nspcc-dev/neo-go/blob/cf4d4a2611209d8d085ccec6e1f1b8956006a577/pkg/vm/stackitem/serialization.go#L144. We use a special stackitem stub of 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 understand your concern, but issues has being there and this pr is not for that purpose, you can have another pr to fix it. for me, no one but hackers will ever see that information. 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. But if we're changing this code anyway, why not to make it work properly? 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.
Cause its not the purpose of this pr, though its easy to update, but that is your idea and your property, and most importantly, its another issue. 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 agree with the fix itself. But then let's not to change this line:
And create the proper issue to improve it. 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. Ref. #3303. |
||
detailedException = string.Concat(detailedException.AsSpan(0, 1021), "..."); | ||
ByteString exceptionRecord = new(Encoding.UTF8.GetBytes(detailedException)); | ||
_snapshot.Put(key, BinarySerializer.Serialize(exceptionRecord, ExecutionEngineLimits.Default with { MaxItemSize = (uint)Settings.Default.MaxStackSize })); | ||
} | ||
return id; | ||
} | ||
|
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.
Thanks @Jim8y. You've create a lot of PRs that makes exception messages much more clear than before. So as this one.
It would be better if we change the magic 1024 to some constant variable like
Default.MaxStackSize/2
or something else.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.
@dusmart Thank you for the recognition, credits of this fix goes to @Hecate2. Need @dusmart your suggestion to decide that value, is Default.MaxStackSize/2 secure enough.
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.
I'm not sure what's the real value of
Default.MaxStackSize
. 1024 and 2048 or any number, they all seems good to me. Maybe @AnnaShaleva has more experience on this settings.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.
@dusmart, to be honest, I'd prefer not mix the real (even invalid) stackitem with some system-created one. See the #3299 (comment).
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.
Settings comes the
config.json
so it can be whatever you want.