Trap OOB on externalcodecopy #272
Conversation
src/eei.cpp
Outdated
| @@ -395,7 +395,8 @@ inline int64_t maxCallGas(int64_t gas) { | |||
| // FIXME: optimise this so no vector needs to be created | |||
| vector<uint8_t> codeBuffer(length); | |||
| size_t numCopied = context->fn_table->copy_code(context, &address, codeOffset, codeBuffer.data(), codeBuffer.size()); | |||
| fill_n(&codeBuffer[numCopied], length - numCopied, 0); | |||
| // Make sure we didn't try to copy beyond the end of the code | |||
| ensureCondition(numCopied == length, InvalidMemoryAccess, "Out of bounds memory copy"); | |||
There was a problem hiding this comment.
Actually can you change the message to Out of bounds (source) memory copy to be in line with the rest? (Please squash it into this)
There was a problem hiding this comment.
What exactly does the "source" and "destination" here mean? I couldn't understand it
There was a problem hiding this comment.
source memory vs. destination memory
There was a problem hiding this comment.
I think I figured out what's bothering me. While the "destination" checks make sense, the "source" checks in the loadMemory methods only effectively check that the offset is non-negative. E.g. on line 899:
ensureCondition((srcOffset + length) >= srcOffset, InvalidMemoryAccess, "Out of bounds (source) memory copy.");
No check is actually made here that we're not attempting to copy beyond the bounds of memory. I added a sample test for this. Do you agree this is an issue? If so I'll fix it and PR.
There was a problem hiding this comment.
It does seem to check for both:
ensureCondition((srcOffset + length) >= srcOffset, InvalidMemoryAccess, "Out of bounds (source) memory copy.");
ensureCondition(src.size() >= (srcOffset + length), InvalidMemoryAccess, "Out of bounds (source) memory copy.");
There was a problem hiding this comment.
Oh you mean loadMemoy. That should check against memory.size().
There was a problem hiding this comment.
Exactly. Okay, I'll fix it, and this, too.
|
Hm, seems that #128 also fixed this same bug. |
|
If that will be merged soon and this is redundant feel free to close this, up to you. |
lrettig
force-pushed
the
extcodecopy-trap-oob
branch
from
1ed2f7d to
048f550
Compare
axic
force-pushed
the
extcodecopy-trap-oob
branch
from
048f550 to
f93e8dd
Compare
Current behavior is to zero pad. Bring behavior in line with codecopy by trapping.
axic
force-pushed
the
extcodecopy-trap-oob
branch
from
f93e8dd to
d7244f3
Compare
No it is just funny we had it for so long, but wasn't noticed properly. |
Per ewasm/tests#34 and #256 (comment) we want codecopy and extcodecopy to trap on OOB rather than zero padding.