Safer ReadVarBytes

[shargon]

No description provided.

[igormcoelho]

If Stream supports seeking, update maximum to maximum seekable data. Nice!

[igormcoelho]

Shargon, I added some tests that help validating this part. Feel free to remove them, if not necessary (but I think they are 😉 )

[igormcoelho]

I think that we never use and don't need so much memory in any Neo process. Perhaps, a lower limit such as 16MB (as used in other methods 0x10000) or half int (~1GB) could be enforced in this case. For example, we could add if( max > 0x10000 ) throw Exception or if( max > Int32.MaxValue / 2 ) throw Exception... if well documented on the method.

[erikzhang]

This can result in the reading of truncated data.

0x08123456

You should throw an exception instead of returning 0x123456.

[shargon]

Now is returned the data truncated, i do this for prevent the logic change

[erikzhang]

But reading truncated data is not the expected behavior of this function and can cause errors elsewhere.

[shargon]

@igormcoelho you left one 00 :P
But this could be a solution, maybe we can check if is higher than the max 0x1000000 (16 Mb)

[igormcoelho]

Truncate or not to truncate? That's the question 🤔 💀
I agree with Erik, it's better an exception, ;f possible. We should create SafeReadBytes function and use it instead of ReadBytes everywhere then.

[erikzhang]

What's the difference between this and the old implemention?

[shargon]

@erikzhang This fix a error mentioned in #Neo-Security channel

[erikzhang]

If reader.BaseStream.CanSeek == false, it is the same as the original implemention. We should limit max.

[erikzhang]

I checked everywhere, the maximum max is 16MB, no problem.

[erikzhang]

So the fix should be in neo-vm.

[erikzhang]

neo/neo/SmartContract/ApplicationEngine.cs

Lines 125 to 132 in f454e98

	case OpCode.PUSHDATA4:
	{
	if (CurrentContext.InstructionPointer + 4 >= CurrentContext.Script.Length)
	return false;
	uint length = CurrentContext.Script.ToUInt32(CurrentContext.InstructionPointer + 1);
	if (length > MaxItemSize) return false;
	return true;
	}

The stack element has been limited to 1MB here.

[shargon]

You are right

[shargon]

Closed because the fix should be on NeoVM