Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Neo Plugin Bug]fix 3296 #3299

Merged
merged 8 commits into from
Jun 11, 2024
Merged

[Neo Plugin Bug]fix 3296 #3299

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a25c723
fix 3296
Jim8y Jun 7, 2024
c183732
Update src/Plugins/ApplicationLogs/Store/LogStorageStore.cs
Jim8y Jun 7, 2024
1378d24
add exception message to the incorrect storeitem
Jim8y Jun 7, 2024
dc0b574
optimize the code to apply Hecate suggestion
Jim8y Jun 7, 2024
612497a
Merge branch 'master' into fix-3296
Jim8y Jun 7, 2024
512d738
revert change to the exception info
Jim8y Jun 7, 2024
ae94667
clean using
Jim8y Jun 7, 2024
e04e1be
Merge branch 'master' into fix-3296
shargon Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/Plugins/ApplicationLogs/Store/LogStorageStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using Neo.SmartContract;
using Neo.VM;
using Neo.VM.Types;
using System.Text;

namespace Neo.Plugins.ApplicationLogs.Store
{
Expand Down Expand Up @@ -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)
Copy link

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.

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Member

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).

Copy link
Member

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.

Settings comes the config.json so it can be whatever you want.

Copy link
Member

Choose a reason for hiding this comment

The 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 Null stackitem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Null or exception both works. Using exception message to explicitly tell people there is an issue.

Copy link
Member

@AnnaShaleva AnnaShaleva Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 Null or ByteString because from the user side it's impossible to distinguish the "good" original stackitem from the result of invalid seialization.

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 InvalidT type which can't be produced by any means other than exception during serialisation. With this special type there's no doubts from the user side whether there was an exception during serialization or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Null or ByteString because from the user side it's impossible to distinguish the "good" original stackitem from the result of invalid seialization.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

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.

Copy link
Member

@AnnaShaleva AnnaShaleva Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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:

_snapshot.Put(key, BinarySerializer.Serialize(StackItem.Null, ExecutionEngineLimits.Default with { MaxItemSize = (uint)Settings.Default.MaxStackSize }));

And create the proper issue to improve it.

Copy link
Member

Choose a reason for hiding this comment

The 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;
}
Expand Down