Implement the translator code in C/C++ #153

Silur · 2018-01-10T14:01:27Z

Part of #4.

evm2wast
TODO stackcheck
TODO tests

axic · 2018-01-10T14:03:41Z

evm2wast.c

+	}
+	return ret;
+}
+int evm2wasm(char *evm_code, size_t len, char *wast_code, size_t wast_size)


This should be called evm2wast. Also I think wast_size should be a pointer updated with the final length.

wast_code must be a pointer of a pointer if you want to use realloc below. Alternatively return the modified pointer (or null) instead of 0/-1.

axic · 2018-01-10T14:03:54Z

evm2wast.c

+			continue;
+		}
+		wast_chunk = gadgets[(int)evm_code[i]];
+		wast_code = realloc(wast_code, strlen(wast_code)+strlen(wast_chunk) + 3);


There are no checks for allocation failures.

axic · 2018-01-10T14:04:45Z

Need to change evm2wasm.cpp to use evm2wast.c.

axic · 2018-01-10T14:07:22Z

evm2wast.h

+#ifndef __EVM2WAST_H
+#define __EVM2WAST_H
+#include <stdlib.h>
+#include "gadgets.h"


Does gadgets needs to be included in the external interface?

Can you remove it in that case?

axic · 2018-01-10T14:07:31Z

evm2wast.h

+#define __EVM2WAST_H
+#include <stdlib.h>
+#include "gadgets.h"
+enum opcodes {


Is this needed in the external interface?

only if you want to access opcodes somewhere after evm2wast output

Can you remove it in that case?

axic · 2018-01-10T14:07:43Z

evm2wast.h

+	STATICAL, REVERT,
+	SELFDESTRUCT = 0XFF
+};
+extern int evm2wasm(char *evm_code, size_t len, char *wast_code, size_t wast_size);


This should be extern "C" {}.

axic · 2018-01-10T16:03:14Z

evm2wast.c

+#include "evm2wast.h"
+#include "gadgets.h"
+#include "util.h"
+#define digits(x) (floor(log10(abs(x))) + 1) 


I like inline functions better because type matching is worse with such complex macros.

For example I get this:

warning: taking the absolute value of unsigned type 'size_t' (aka 'unsigned long') has no effect [-Wabsolute-value] note: remove the call to 'abs' since unsigned values cannot be negative

axic · 2018-01-10T17:08:01Z

My commit collapsed all the comments, but most of them are still relevant - please check them out.

axic · 2018-01-10T17:33:56Z

evm2wast.c

 	}
 	char *with_segments = assemble_segments(jump_segments, 4);
 	wast_code = realloc(wast_code, strlen(wast_code) + strlen(with_segments));
+	if(wast_code == 0) goto err;


Note: realloc doesn't free the original memory space on failure.

only if the first param and the return value is different
b = realloc(a, size) will leave a untouched and set b to 0.

realloc has no understanding what the lvalue is.

See the man pages:

For realloc(), the input pointer is still valid if reallocation failed. For reallocf(), the input pointer will have been freed if reallocation failed.

The realloc() function returns a pointer to the newly allocated memory, which is suitably aligned for any built-in type and may be different from ptr, or NULL if the request fails .... If realloc() fails, the original block is left untouched; it is not freed or moved.

implementation dependent but as the README.md states we go with glibc

I do not follow.

For one, doesn't matter what the README says, you cannot claim "go with glibc" since it will definitely not use glibc when compiling as a contract.

Second, I think it is clear what the manual is saying. The input pointer passed to realloc is not freed upon failure.

axic · 2018-01-12T11:24:55Z

Please do not remove the Javascript code and rebase this against master.

axic · 2018-01-12T12:26:51Z

Please write a shell script (and CI entry) which

takes a directory,
compiles each file (EVM bytecode in those files) with evm2wasm.js and evm2wasm.cpp,
and compares them.

They should be identical.

Then we can just rely on @cdetrio's tests on evm2wasm.js. Later we can run @cdetrio's tests with running each twice and transpiling them with both evm2wasm.js and evm2wasm.cpp.

axic · 2018-01-12T13:27:46Z

Include gadgets.h (and anything else generated) in .gitignore.

axic · 2018-01-12T13:32:39Z

Oh we need to include running gen_gadgets.sh in cmake, @chfast if you have a chance sometime later.

axic · 2018-01-12T13:37:50Z

gen_gadgets.sh

@@ -0,0 +1,99 @@
+cat <<EOF > gadgets.h


I wonder if this should be a c file instead? I'm not a big fan including giant data structures in headers because they are pulled in every time the header is included.

Probably generating gadgets.c with the content and gadgets.h with the reference would be a good idea.

The header can be something like:

#ifndef __EVM2WASM_GADGETS_H #define __EVM2WASM_GADGETS_H /* Declared in gadgets.c */ char *gadgets[256]; #endif

axic · 2018-01-12T13:39:20Z

evm2wast.c

+	return wasm;
+err:
+	free(brtable);
+	free(wasm);


This could result in freeing NULL in the case of brtable or wasm allocation fails.

axic · 2018-01-12T13:39:31Z

evm2wast.c

+err:
+	free(brtable);
+	free(wasm);
+	free(wasm);


This is a double free here.

axic · 2018-01-12T13:51:10Z

Do you want to leave a bit more space in the code, e.g. between if ( and in arithmetics?

axic · 2018-01-12T13:52:08Z

evm2wast.c

+#include <stdarg.h>
+#include "evm2wast.h"
+#include "gadgets.h"
+int digits(unsigned long x) { return (floor(log10(abs(x))) + 1); }


Should be static if we need this helper. Also why place it above the global variables and not next to the functions?

axic · 2018-01-12T13:53:28Z

evm2wast.h

+
+enum op_nums {
+	STOP = 0x00,
+	ADD, MUL, SUB, DIV, SDIV, MOD, SMOD,


Do you want to keep all these as one-per-line? Makes new additions much easier to spot.

axic · 2018-01-12T13:53:55Z

evm2wast.h

+	STOP = 0x00,
+	ADD, MUL, SUB, DIV, SDIV, MOD, SMOD,
+	ADDMOD, MULMOD, EXP, SIGNEXTEND,
+	LT = 0X10, GT, SLT, SGT, EQ, ISZERO,


Can you keep using lowercase x in the hex values?

axic · 2018-01-13T15:46:53Z

@Silur can you squash all this branch down to a single commit and rebase it? And make sure afterwards not to touch the javascript files and just exclude the old makefile.

chfast · 2018-01-13T15:57:11Z

This is minimal change to run gen_gadgets.sh from cmake:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 42d408f..6f3cc96 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -11,9 +11,16 @@ project(evm2wasm)
 
 include(ProjectBinaryen)
 
+add_custom_target(gen_gadgets
+    ${PROJECT_SOURCE_DIR}/gen_gadgets.sh
+    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
+)
+
 add_executable(evm2wasm
     evm2wasm.cpp
     evm2wast.c
+    gadgets.c
 )
 
+add_dependencies(evm2wasm gen_gadgets)
 target_link_libraries(evm2wasm PRIVATE binaryen::binaryen)
\ No newline at end of file
diff --git a/gen_gadgets.sh b/gen_gadgets.sh
index 46b2c72..520411a 100755
--- a/gen_gadgets.sh
+++ b/gen_gadgets.sh
@@ -1,3 +1,5 @@
+#!/bin/bash
+
 cat <<EOF > gadgets.c
 #ifndef __EVM2WASM_GADGETS_H
 #define __EVM2WASM_GADGETS_H

axic · 2018-01-14T02:11:15Z

evm2wast.c

+	return wasm;
+err:
+	perror("Falied to allocate memory for assemble_segments");
+	return 0;


Memory isn't freed in case of an error?

axic · 2018-01-14T17:14:16Z

evm2wasm.cpp

    wasm::Fatal() << "error in parsing input";
-  }
-
-  // FIXME: perhaps call validate() here?


Why remove this?

axic · 2018-01-14T17:14:50Z

evm2wasm.cpp

+	fseek(fd, 0, SEEK_END);
+	size_t offset = ftell(fd);
+	rewind(fd);
+	char *code = (char*)malloc(8192);


If offset is the file size, then why not use it here?

oh yea I forgot to use that :D

axic · 2018-01-14T17:23:30Z

evm2wast.c

-	char *check = "";
-	char *template = "";
+	char *check = malloc(1);
+	char *template = malloc(0);


I am not really fond of malloc(1) and malloc(0) but realloc(NULL, size) is perfectly valid so these can be set to NULL

can't because below we use strlen on them what will cause a segfault

axic · 2018-01-14T17:28:27Z

CMakeLists.txt

 add_executable(evm2wasm
    evm2wasm.cpp
+    evm2wast.c
+		gadgets.c


Indentation is off

axic · 2018-01-14T19:58:01Z

Please read #153 (comment) and please mark every char* parameter as const which is not supposed to be modified.

axic · 2018-01-15T18:24:48Z

libs/evm2wasm/evm2wast.c

+					char *bytes = padleft(evm_code+1, evm_code[i]-0x5f, 32);
+					i+=(size_t)evm_code[i]-0x5f;
+					if(!bytes) goto err;
+					int bytes_rounded = (int)ceil((double)(evm_code[i-1]/8));


If you can, please avoid using float/double and math ops.

(evm_code[i - 1] + 7) / 8 should have the same effect.

axic · 2018-01-15T18:31:18Z

libs/evm2wasm/evm2wast.c

+	}
+	return ret;
+}
+static int64_t bytes2long(char *bytes)


bytes can be const here

axic · 2018-01-15T18:31:31Z

libs/evm2wasm/evm2wast.c

+	if(!ret)
+	{
+		perror("allocation error ");
+		return 0;


This isn't really freeing the memory?

axic · 2018-01-15T18:33:10Z

libs/evm2wasm/evm2wast.c

+	return 0;
+}
+
+static char *add_stack_check(char *segment)


There is realloc bellow and this pointer is lost for the caller.

axic · 2018-01-15T18:34:11Z

libs/evm2wasm/evm2wast.c

+	stack_delta = 0;
+	return segment;
+err:
+	free(template);


String literals (as they may be placed into read-only sections) cannot be freed. You assign string literals to template above.

axic · 2018-01-15T18:34:36Z

libs/evm2wasm/evm2wast.c

+	}
+	return --i;
+}
+char *build_module(char *wast)


wast can be const

axic · 2018-01-15T19:39:05Z

libs/evm2wasm/evm2wast.c

+	return ret;
+}*/
+
+static int index_of(char **table, char *elem, int len)


elem can be const

axic · 2018-01-15T19:40:02Z

libs/evm2wasm/evm2wast.c

+static char *add_stack_check(char **segment)
+{
+	char *check = malloc(1);
+	char *template = malloc(1);


These two are never freed.

axic · 2018-01-16T15:08:17Z

Please do a rebase and perhaps squash it down?

axic · 2018-01-16T16:47:28Z

libs/evm2wasm/evm2wast.c

-	sprintf(*wast, "(call $useGas (i64.const %ld)) %s", gas_count, *segment);
-	*segment = "";
+	sprintf(*wast, "%s (call $useGas (i64.const %ld)) %s", *wast, gas_count, *segment);
+	free(*segment);


After this free the caller will have an invalid pointer.

axic · 2018-01-16T18:04:56Z

libs/evm2wasm/evm2wast.c

 	}
 	sprintf(*wast, "%s (call $useGas (i64.const %ld)) %s", *wast, gas_count, *segment);
-	free(*segment);
+	*segment = "";


This will put a literal into a pointer which is freed later on and throws away the pointer which needs to be freed (memory leak).

axic · 2018-01-24T23:39:51Z

I'd suggest try using cppcheck (or clang's static analyzer, but that is a bigger tool), it can find memory misuse issues too.

axic · 2018-01-25T00:29:19Z

libs/evm2wasm/evm2wast.c

+			case PUSH29:
+			case PUSH30:
+			case PUSH31:
+			case PUSH32:


Could use PUSH1 .. PUSH32 in gnuc99 :)

axic · 2018-01-25T00:40:40Z

libs/evm2wasm/gen_gadgets.sh

@@ -0,0 +1,55 @@
+#!/bin/bash
+cstr () { 
+sed 's/;;.*//g' $1 2>/dev/null | sed '$!s/$/\\n\\/' | sed 's/"/\\"/g' 


Please fix this so it can handle empty trailing new lines.

chfast · 2018-01-25T08:15:34Z

If you still have memory issues, build with Address Sanitizer (GCC, clang): -fsanitize=address. This is very good tool to know.

Reimplement the translator in C minor memory fixes shitty size fixes shitty size fixes remove debug printf

axic · 2018-01-25T23:04:18Z

I don't think you're actually doing a rebase because running a rebase on this results in a lot of conflicts also there seem to be quite a few back and forth changes.

axic · 2018-01-25T23:12:07Z

Pushed a rebase into the crewrite branch of the master repo. Please check it out.

Also be reenabling the compiler warnings, it spits out a lot of potential cases for memory corruption (and their potential solutions):

/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:465:24: warning: passing 'const char *' to parameter of type 'char *' discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
                                i = next_jump_dest(evm_code, len, i);
                                                   ^~~~~~~~
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:370:36: note: passing argument to parameter 'evm_code' here
static size_t next_jump_dest(char *evm_code, size_t len, size_t i)
                                   ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:491:17: warning: incompatible pointer types passing 'char *' to parameter of type 'char **'; take the
      address with & [-Wincompatible-pointer-types]
                                end_segment(wast_code, segment);
                                            ^~~~~~~~~
                                            &
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:362:39: note: passing argument to parameter 'wast' here
static inline void end_segment(char **wast, char **segment)
                                      ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:491:28: warning: incompatible pointer types passing 'char *' to parameter of type 'char **'; take the
      address with & [-Wincompatible-pointer-types]
                                end_segment(wast_code, segment);
                                                       ^~~~~~~
                                                       &
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:362:52: note: passing argument to parameter 'segment' here
static inline void end_segment(char **wast, char **segment)
                                                   ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:499:18: warning: incompatible pointer types passing 'char *' to parameter of type 'char **'; take the
      address with & [-Wincompatible-pointer-types]
                                add_metering(wast_code, segment);
                                             ^~~~~~~~~
                                             &
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:349:40: note: passing argument to parameter 'wast' here
static inline void add_metering(char **wast, char **segment)
                                       ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:499:29: warning: incompatible pointer types passing 'char *' to parameter of type 'char **'; take the
      address with & [-Wincompatible-pointer-types]
                                add_metering(wast_code, segment);
                                                        ^~~~~~~
                                                        &
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:349:53: note: passing argument to parameter 'segment' here
static inline void add_metering(char **wast, char **segment)
                                                    ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:583:28: warning: passing 'const char *' to parameter of type 'char *' discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
                                        char *bytes = padleft(evm_code+1, evm_code[i]-0x5f, 32);
                                                              ^~~~~~~~~~
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:234:28: note: passing argument to parameter 'bytes' here
static char *padleft(char *bytes, int curr_len, int len)
                           ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:596:32: warning: implicit declaration of function 'slice' is invalid in C99
      [-Wimplicit-function-declaration]
                                                int64_t i64 = bytes2long(slice(bytes+(q*8), q*8+8));
                                                                         ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:596:32: warning: incompatible integer to pointer conversion passing 'int' to parameter of type
      'const char *' [-Wint-conversion]
                                                int64_t i64 = bytes2long(slice(bytes+(q*8), q*8+8));
                                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:224:39: note: passing argument to parameter 'bytes' here
static int64_t bytes2long(const char *bytes)
                                      ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:599:49: warning: format specifies type 'long' but the argument has type 'int64_t' (aka 'long long')
      [-Wformat]
                                                sprintf(push, "%s (i64.const %ld)", push, i64);
                                                                             ~~~          ^~~
                                                                             %lld
/usr/include/secure/_stdio.h:47:56: note: expanded from macro 'sprintf'
  __builtin___sprintf_chk (str, 0, __darwin_obsz(str), __VA_ARGS__)
                                                       ^~~~~~~~~~~
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:616:25: warning: passing 'const char *' to parameter of type 'char *' discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
                                        i = next_jump_dest(evm_code, len, i);
                                                           ^~~~~~~~
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:370:36: note: passing argument to parameter 'evm_code' here
static size_t next_jump_dest(char *evm_code, size_t len, size_t i)
                                   ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:629:25: warning: passing 'const char *' to parameter of type 'char *' discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
                                        i = next_jump_dest(evm_code, len, i);
                                                           ^~~~~~~~
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:370:36: note: passing argument to parameter 'evm_code' here
static size_t next_jump_dest(char *evm_code, size_t len, size_t i)
                                   ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:639:24: warning: passing 'const char *' to parameter of type 'char *' discards qualifiers
      [-Wincompatible-pointer-types-discards-qualifiers]
                                i = next_jump_dest(evm_code, len, i);
                                                   ^~~~~~~~
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:370:36: note: passing argument to parameter 'evm_code' here
static size_t next_jump_dest(char *evm_code, size_t len, size_t i)
                                   ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:672:14: warning: incompatible pointer types passing 'char *' to parameter of type 'char **'; take the
      address with & [-Wincompatible-pointer-types]
        end_segment(wast_code, segment);
                    ^~~~~~~~~
                    &
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:362:39: note: passing argument to parameter 'wast' here
static inline void end_segment(char **wast, char **segment)
                                      ^
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:672:25: warning: incompatible pointer types passing 'char *' to parameter of type 'char **'; take the
      address with & [-Wincompatible-pointer-types]
        end_segment(wast_code, segment);
                               ^~~~~~~
                               &
/Users/alex/Projects/evm2wasm/libs/evm2wasm/evm2wast.c:362:52: note: passing argument to parameter 'segment' here
static inline void end_segment(char **wast, char **segment)
                                                   ^
141 warnings generated.

Silur · 2018-01-26T00:47:47Z

@chfast I already hooked up asan on my copy just didn't include it in cmakefiles.
also dafuq hapenned seems like several commits are missing now, my reflog is empty

axic · 2018-03-26T22:43:03Z

Superseded by #208.

axic reviewed Jan 10, 2018

View reviewed changes

Silur force-pushed the crewrite branch from fde20e7 to 6e36e9f Compare January 12, 2018 13:18

axic reviewed Jan 12, 2018

View reviewed changes

evm2wast.c Outdated

err:

free(brtable);

free(wasm);

free(wasm);

Copy link

Member

axic Jan 12, 2018

Choose a reason for hiding this comment

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

This is a double free here.

axic reviewed Jan 12, 2018

View reviewed changes

axic requested a review from chfast January 12, 2018 19:32

axic changed the title ~~Crewrite~~ Implement the translator code in C/C++ Jan 13, 2018

axic reviewed Jan 14, 2018

View reviewed changes

evm2wasm.cpp Outdated

wasm::Fatal() << "error in parsing input";

}

// FIXME: perhaps call validate() here?

Copy link

Member

axic Jan 14, 2018

Choose a reason for hiding this comment

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

Why remove this?

axic reviewed Jan 14, 2018

View reviewed changes

CMakeLists.txt

add_executable(evm2wasm

evm2wasm.cpp

evm2wast.c

gadgets.c

Copy link

Member

axic Jan 14, 2018

Choose a reason for hiding this comment

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

Indentation is off

axic force-pushed the crewrite branch from 09dd8b7 to 556c9fa Compare January 14, 2018 19:43