From 507bdda25a6292ef1a5b2aa2972b4a17c8d0cead Mon Sep 17 00:00:00 2001
From: Mamy Ratsimbazafy <mratsim@users.noreply.github.com>
Date: Thu, 17 May 2018 20:59:17 +0200
Subject: [PATCH] Catch stack underflow (#33)

* Stack underflow: Add failing test to catch #31

* ensurePop, use proc instead of template + add comment for future refactoring of popInternal

* Check stack underflows before popping values

* run json tests again
---
 VMTests.md           |  6 +++---
 src/vm/stack.nim     | 27 ++++++++++++++++-----------
 tests/test_stack.nim |  9 +++++++++
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/VMTests.md b/VMTests.md
index 540ba3e0e..6ad20d4b0 100644
--- a/VMTests.md
+++ b/VMTests.md
@@ -132,7 +132,7 @@ VMTests
 + mul5.json                                                       OK
 + mul6.json                                                       OK
 + mul7.json                                                       OK
-- mulUnderFlow.json                                               Fail
++ mulUnderFlow.json                                               OK
 + mulmod0.json                                                    OK
 + mulmod1.json                                                    OK
 + mulmod1_overflow.json                                           OK
@@ -163,7 +163,7 @@ VMTests
 + sdivByZero0.json                                                OK
 + sdivByZero1.json                                                OK
 + sdivByZero2.json                                                OK
-- sdiv_dejavu.json                                                Fail
++ sdiv_dejavu.json                                                OK
 + sdiv_i256min.json                                               OK
 + sdiv_i256min2.json                                              OK
 + sdiv_i256min3.json                                              OK
@@ -198,7 +198,7 @@ VMTests
 + sub3.json                                                       OK
 + sub4.json                                                       OK
 ```
-OK: 186/195 Fail: 8/195 Skip: 1/195
+OK: 188/195 Fail: 6/195 Skip: 1/195
 ## vmBitwiseLogicOperation
 ```diff
 + and0.json                                                       OK
diff --git a/src/vm/stack.nim b/src/vm/stack.nim
index 359282c5c..111b0ac9c 100644
--- a/src/vm/stack.nim
+++ b/src/vm/stack.nim
@@ -75,28 +75,31 @@ proc push*(stack: var Stack, value: Bytes) =
   stack.values.add(value.toType(UInt256))
 
 proc internalPop(stack: var Stack, numItems: int): seq[UInt256] =
-  if len(stack) < numItems:
-    result = @[]
-  else:
-    result = stack.values[^numItems .. ^1]
-    stack.values = stack.values[0 ..< ^numItems]
+  # TODO: it is very inefficient to allocate a seq
+  assert  numItems <= stack.len
+  result = stack.values[^numItems .. ^1]
+  stack.values = stack.values[0 ..< ^numItems]
 
 proc internalPop(stack: var Stack, numItems: int, T: typedesc): seq[T] =
+  # TODO: it is very inefficient to allocate a seq
+
+  assert  numItems <= stack.len
   result = @[]
-  if len(stack) < numItems:
-    return
 
   for z in 0 ..< numItems:
     var value = stack.values.pop()
     result.add(toType(value, T))
 
-template ensurePop(elements: untyped, a: untyped): untyped =
-  if len(`elements`) < `a`:
-    raise newException(InsufficientStack, "No stack items")
+proc ensurePop(elements: seq|Stack, a: int) =
+  let num = elements.len
+  let expected = a
+  if num < expected:
+    raise newException(InsufficientStack,
+      &"Stack underflow: expected {expected} elements, got {num} instead.")
 
 proc popInt*(stack: var Stack): UInt256 =
+  ensurePop(stack, 1)
   var elements = stack.internalPop(1, UInt256)
-  ensurePop(elements, 1)
   result = elements[0]
 
 macro internalPopTuple(numItems: static[int]): untyped =
@@ -110,6 +113,8 @@ macro internalPopTuple(numItems: static[int]): untyped =
   result = quote:
     proc `name`*(`stackNode`: var Stack, `t`: typedesc): `typ`
   result[^1] = nnkStmtList.newTree()
+  result[^1].add quote do:
+    ensurePop(`stackNode`, `numItems`)
   for z in 0 ..< numItems:
     var zNode = newLit(z)
     var element = quote:
diff --git a/tests/test_stack.nim b/tests/test_stack.nim
index cb128599e..c5ae63f79 100644
--- a/tests/test_stack.nim
+++ b/tests/test_stack.nim
@@ -93,3 +93,12 @@ suite "stack":
     var stack = newStack()
     expect(InsufficientStack):
       stack.dup(0)
+
+  test "binary operations raises InsufficientStack appropriately":
+    # https://github.com/status-im/nimbus/issues/31
+    # ./tests/fixtures/VMTests/vmArithmeticTest/mulUnderFlow.json
+
+    var stack = newStack()
+    stack.push(123)
+    expect(InsufficientStack):
+      discard stack.popInt(2)