Skip to content

explicitly duplicate bignumbers on stack #733

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

Merged
merged 3 commits into from
Apr 30, 2020
Merged

explicitly duplicate bignumbers on stack #733

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aea0c04
explicitly duplicate bignumbers on stack
jochem-brouwer Apr 27, 2020
28991cd
vm: add test to check for internally editing stack items
jochem-brouwer Apr 28, 2020
4488760
Merge branch 'master' into fix-bn-dup
evertonfraga Apr 30, 2020
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
2 changes: 1 addition & 1 deletion packages/vm/lib/evm/stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,6 @@ export default class Stack {
}

const i = this._store.length - position
this.push(this._store[i])
this.push(this._store[i].clone())
}
}
48 changes: 48 additions & 0 deletions packages/vm/tests/api/evm/stack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
const tape = require('tape')
const BN = require('bn.js')
const Stack = require('../../../dist/evm/stack').default
const VM = require('../../../dist/index').default
const { createAccount } = require('../utils')

tape('Stack', t => {
t.test('should be empty initially', st => {
Expand Down Expand Up @@ -111,4 +113,50 @@ tape('Stack', t => {
st.throws(() => s.push(max.addn(1)))
st.end()
})

t.test('stack items should not change if they are DUPed', async st => {
const caller = Buffer.from('00000000000000000000000000000000000000ee', 'hex')
const addr = Buffer.from('00000000000000000000000000000000000000ff', 'hex')
const key = new BN(0).toArrayLike(Buffer, 'be', 32)
const vm = new VM()
const account = createAccount('0x00', '0x00')
const code = "60008080808060013382F15060005260206000F3"
const expectedReturnValue = new BN(0).toArrayLike(Buffer, 'be', 32)
/*
code: remarks: (top of the stack is at the zero index)
PUSH1 0x00
DUP1
DUP1
DUP1
DUP1
PUSH1 0x01
CALLER
DUP3
CALL stack: [0, CALLER, 1, 0, 0, 0, 0, 0]
POP pop the call result (1)
PUSH1 0x00
MSTORE we now expect that the stack (prior to MSTORE) is [0, 0]
PUSH1 0x20
PUSH1 0x00
RETURN stack: [0, 0x20] (we thus return the stack item which was originally pushed as 0, and then DUPed)
*/
await vm.stateManager.putAccount(addr, account)
await vm.stateManager.putContractCode(addr, Buffer.from(code, 'hex'))
const runCallArgs = {
caller: caller,
gasLimit: new BN(0xffffffffff),
to: addr,
value: new BN(1)
}
try {
const res = await vm.runCall(runCallArgs)
const executionReturnValue = res.execResult.returnValue
st.assert(executionReturnValue.equals(expectedReturnValue))
st.end()
} catch(e) {
st.fail(e.message)
}
})


Copy link
Member

@holgerd77 holgerd77 Apr 28, 2020

Choose a reason for hiding this comment

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

Thanks, that's great! 😄

Ah, yeah, I see what you mean ("a bit out of place"), lol 😜, but anyhow, doesn't hurt or blow things up either, at least would be my stance here on this.

Copy link
Member Author

@jochem-brouwer jochem-brouwer Apr 28, 2020

Choose a reason for hiding this comment

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

I don't really see how to make it much compacter either. For now, it seems like only this gas stipend thing messes up the stack as I don't see any other potentially stack-editing operations in opFns via BN.js i* arithmetic when glancing over the file.

})