[Tarantool-patches] [PATCH 0/2] Fix embedded bytecode loader

[Tarantool-patches] [PATCH 0/2] Fix embedded bytecode loader

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Tarantool PR: https://github.com/tarantool/tarantool/pull/8913
Related issue: https://github.com/LuaJIT/LuaJIT/issues/549
Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-549-fix-embedded-bytecode-loader

Mike Pall (2):
  Fix embedded bytecode loader.
  Followup fix for embedded bytecode loader.

 src/lj_bcread.c                               |  10 +-
 src/lj_lex.c                                  |   7 +
 src/lj_lex.h                                  |   1 +
 test/tarantool-c-tests/lj-549-lua_load.test.c | 146 ++++++++++++++++++
 4 files changed, 159 insertions(+), 5 deletions(-)
 create mode 100644 test/tarantool-c-tests/lj-549-lua_load.test.c

--
2.34.1

[Tarantool-patches] [PATCH 1/2] Fix embedded bytecode loader.

[Sergey Bronnikov via Tarantool-patches]

From: sergeyb@tarantool.org

(cherry-picked from commit 820339960123dc78a7ce03edf53fcf4fdae0e55d)

Original problem is specific for x32 and is as follows: when a chunk
with bytecode library is loaded into memory, and the address is higher
than 0x80000100, the LexState->pe, that contains an address of the end
of bytecode chunk in the memory, will wrap around and become smaller
than address in LexState->p, that contains an address of the beginning
of bytecode chunk in the memory. In bcread_fill() called by
bcread_want(), memcpy() is called with a very large size and causes bus
error on x86 and segmentation fault on ARM android.

The problem cannot be reproduced on platforms supported by Tarantool
(ARM64, x86_64), so test doesn't reproduce a problem without a patch and
tests patch partially.

Sergey Bronnikov:
* added the description
---
 src/lj_bcread.c | 10 +++++-----
 src/lj_lex.c    |  6 ++++++
 src/lj_lex.h    |  1 +
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/lj_bcread.c b/src/lj_bcread.c
index f6c7ad25..315ad4d8 100644
--- a/src/lj_bcread.c
+++ b/src/lj_bcread.c
@@ -79,6 +79,7 @@ static LJ_NOINLINE void bcread_fill(LexState *ls, MSize len, int need)
       ls->c = -1;  /* Only bad if we get called again. */
       break;
     }
+    if (sz >= LJ_MAX_BUF - n) lj_err_mem(ls->L);
     if (n) {  /* Append to buffer. */
       n += (MSize)sz;
       p = lj_buf_need(&ls->sb, n < len ? len : n);
@@ -90,20 +91,20 @@ static LJ_NOINLINE void bcread_fill(LexState *ls, MSize len, int need)
       ls->p = buf;
       ls->pe = buf + sz;
     }
-  } while (ls->p + len > ls->pe);
+  } while ((MSize)(ls->pe - ls->p) < len);
 }
 
 /* Need a certain number of bytes. */
 static LJ_AINLINE void bcread_need(LexState *ls, MSize len)
 {
-  if (LJ_UNLIKELY(ls->p + len > ls->pe))
+  if (LJ_UNLIKELY((MSize)(ls->pe - ls->p) < len))
     bcread_fill(ls, len, 1);
 }
 
 /* Want to read up to a certain number of bytes, but may need less. */
 static LJ_AINLINE void bcread_want(LexState *ls, MSize len)
 {
-  if (LJ_UNLIKELY(ls->p + len > ls->pe))
+  if (LJ_UNLIKELY((MSize)(ls->pe - ls->p) < len))
     bcread_fill(ls, len, 0);
 }
 
@@ -463,8 +464,7 @@ GCproto *lj_bcread(LexState *ls)
     setprotoV(L, L->top, pt);
     incr_top(L);
   }
-  if ((int32_t)(2*(uint32_t)(ls->pe - ls->p)) > 0 ||
-      L->top-1 != bcread_oldtop(L, ls))
+  if ((ls->pe != ls->p && !ls->endmark) || L->top-1 != bcread_oldtop(L, ls))
     bcread_error(ls, LJ_ERR_BCBAD);
   /* Pop off last prototype. */
   L->top--;
diff --git a/src/lj_lex.c b/src/lj_lex.c
index 52856912..82e4ba6f 100644
--- a/src/lj_lex.c
+++ b/src/lj_lex.c
@@ -48,6 +48,11 @@ static LJ_NOINLINE LexChar lex_more(LexState *ls)
   size_t sz;
   const char *p = ls->rfunc(ls->L, ls->rdata, &sz);
   if (p == NULL || sz == 0) return LEX_EOF;
+  if (sz >= LJ_MAX_BUF) {
+    if (sz != ~(size_t)0) lj_err_mem(ls->L);
+    sz = ~(uintptr_t)0 - (uintptr_t)p;
+    ls->endmark = 1;
+  }
   ls->pe = p + sz;
   ls->p = p + 1;
   return (LexChar)(uint8_t)p[0];
@@ -406,6 +411,7 @@ int lj_lex_setup(lua_State *L, LexState *ls)
   ls->lookahead = TK_eof;  /* No look-ahead token. */
   ls->linenumber = 1;
   ls->lastline = 1;
+  ls->endmark = 0;
   lex_next(ls);  /* Read-ahead first char. */
   if (ls->c == 0xef && ls->p + 2 <= ls->pe && (uint8_t)ls->p[0] == 0xbb &&
       (uint8_t)ls->p[1] == 0xbf) {  /* Skip UTF-8 BOM (if buffered). */
diff --git a/src/lj_lex.h b/src/lj_lex.h
index 33fa8657..38d28533 100644
--- a/src/lj_lex.h
+++ b/src/lj_lex.h
@@ -73,6 +73,7 @@ typedef struct LexState {
   BCInsLine *bcstack;	/* Stack for bytecode instructions/line numbers. */
   MSize sizebcstack;	/* Size of bytecode stack. */
   uint32_t level;	/* Syntactical nesting level. */
+  int endmark;		/* Trust bytecode end marker, even if not at EOF. */
 } LexState;
 
 LJ_FUNC int lj_lex_setup(lua_State *L, LexState *ls);
-- 
2.34.1

[Tarantool-patches] [PATCH 2/2] Followup fix for embedded bytecode loader.

[Sergey Bronnikov via Tarantool-patches]

From: sergeyb@tarantool.org

(cherry-picked from commit e49863eda13d095b1a78fd4ca0fd3a6a9a17d782)

Sergey Bronnikov:
  * added the partial test for this and a previous patch
---
 src/lj_lex.c                                  |   1 +
 test/tarantool-c-tests/lj-549-lua_load.test.c | 146 ++++++++++++++++++
 2 files changed, 147 insertions(+)
 create mode 100644 test/tarantool-c-tests/lj-549-lua_load.test.c

diff --git a/src/lj_lex.c b/src/lj_lex.c
index 82e4ba6f..161d862e 100644
--- a/src/lj_lex.c
+++ b/src/lj_lex.c
@@ -51,6 +51,7 @@ static LJ_NOINLINE LexChar lex_more(LexState *ls)
   if (sz >= LJ_MAX_BUF) {
     if (sz != ~(size_t)0) lj_err_mem(ls->L);
     sz = ~(uintptr_t)0 - (uintptr_t)p;
+    if (sz >= LJ_MAX_BUF) sz = LJ_MAX_BUF-1;
     ls->endmark = 1;
   }
   ls->pe = p + sz;
diff --git a/test/tarantool-c-tests/lj-549-lua_load.test.c b/test/tarantool-c-tests/lj-549-lua_load.test.c
new file mode 100644
index 00000000..4fb144cf
--- /dev/null
+++ b/test/tarantool-c-tests/lj-549-lua_load.test.c
@@ -0,0 +1,146 @@
+#include <assert.h>
+#include <stdint.h>
+#include <stddef.h>
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <lua.h>
+#include <lualib.h>
+#include <lauxlib.h>
+
+#include "test.h"
+#include "utils.h"
+
+/* Need for skipcond. */
+#include "lj_arch.h"
+
+/* Defined in lj_def.h. */
+#define LJ_MAX_MEM32    0x7fffff00      /* Max. 32 bit memory allocation. */
+#define LJ_MAX_BUF      LJ_MAX_MEM32    /* Max. buffer length. */
+
+#define UNUSED(x) ((void)(x))
+
+/**
+ * Array with bytecode was generated using commands below:
+ *
+ * cat << EOF > a.lua
+ * local a = 1
+ * EOF
+ * luajit -b a.lua a.h
+ */
+#define luaJIT_BC_sample_SIZE 22
+static const unsigned char luaJIT_BC_sample[] = {
+27,76,74,2,2,15,2,0,1,0,0,0,2,41,0,1,0,75,0,1,0,0
+};
+
+/**
+ * Function generates a huge chunk with "bytecode" with a size bigger than
+ * LJ_MAX_BUF. Generated chunk must enable endmark in a Lex state.
+ */
+static const char *
+bc_reader_with_endmark(lua_State *L, void *data, size_t *size)
+{
+	UNUSED(data);
+	static char *bc_chunk = NULL;
+	free(bc_chunk);
+
+	int bc_chunk_size = (size_t)0;
+	bc_chunk = malloc(bc_chunk_size);
+	assert(bc_chunk != NULL);
+
+	/**
+	 * Put a chunk with a valid bytecode to the beginning of allocated region.
+	 * lua_load automatically detects whether the chunk is text or binary,
+	 * and loads it accordingly. We need a trace for bytecode input,
+	 * so it is necessary to deceive a check in lj_lex_setup, that makes a
+	 * sanity check and detects whether input is bytecode or text by first char.
+	 * Strictly speaking it is enough to put LUA_SIGNATURE[0] as a first
+	 * symbol in produced chunk.
+	 */
+	memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE);
+
+	*size = bc_chunk_size;
+
+	return bc_chunk;
+}
+
+static int bc_loader_with_endmark(void *test_state)
+{
+	lua_State *L = test_state;
+	void *ud = NULL;
+	int res = lua_load(L, bc_reader_with_endmark, ud, "endmark");
+
+	/* Make sure we passed condition with lj_err_mem in a function lex_more. */
+	assert_true(res != LUA_ERRMEM);
+
+	return TEST_EXIT_SUCCESS;
+}
+
+enum bc_emission_state {
+	EMIT_BC,
+	EMIT_EOF,
+};
+
+typedef struct {
+	enum bc_emission_state state;
+} dt;
+
+/**
+ * Function returns bytecode chunk on the first call and NULL and size equals
+ * to zero on the second call. Triggers END_OF_STREAM flag in a function
+ * lex_more.
+ */
+static const char *
+bc_reader_with_eof(lua_State *L, void *data, size_t *size)
+{
+	UNUSED(data);
+	UNUSED(L);
+	dt *test_data = (dt *)data;
+	if (test_data->state == EMIT_EOF) {
+		*size = 0;
+		return NULL;
+	}
+
+	char *bc_chunk = malloc(luaJIT_BC_sample_SIZE);
+	memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE);
+	*size = luaJIT_BC_sample_SIZE;
+
+	return bc_chunk;
+}
+
+static int bc_loader_with_eof(void *test_state)
+{
+	lua_State *L = test_state;
+	dt test_data = {0};
+	test_data.state = EMIT_BC;
+	int res = lua_load(L, bc_reader_with_eof, &test_data, "eof");
+	assert_true(res = LUA_ERRSYNTAX);
+	if (res == LUA_OK) {
+		lua_pcall(L, 0, 0, 0);
+	}
+
+	return TEST_EXIT_SUCCESS;
+}
+
+int main(void)
+{
+	if (LJ_GC64 || !LUAJIT_ARCH_X64 || !LJ_TARGET_LINUX)
+		/**
+		 * lua_load source code is common on all platforms,
+		 * when bytecode is not portable.
+		 * So test runs on Linux/x86_64 only and skipped on other
+		 * platforms.
+		 */
+		return skip_all("Enabled on Linux/x86_64 with disabled GC64");
+
+	lua_State *L = utils_lua_init();
+	const struct test_unit tgroup[] = {
+		test_unit_def(bc_loader_with_endmark),
+		test_unit_def(bc_loader_with_eof)
+	};
+
+	const int test_result = test_run_group(tgroup, L);
+	utils_lua_close(L);
+	return test_result;
+}
-- 
2.34.1

Re: [Tarantool-patches] [PATCH 1/2] Fix embedded bytecode loader.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Tue, Jul 25, 2023 at 07:36:24PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> From: sergeyb@tarantool.org
> 
> (cherry-picked from commit 820339960123dc78a7ce03edf53fcf4fdae0e55d)
> 
> Original problem is specific for x32 and is as follows: when a chunk
Typo: s/Original/The original/
Typo: s/for x32/to x32/
> with bytecode library is loaded into memory, and the address is higher
Typo: s/with bytecode/with a bytecode/
> than 0x80000100, the LexState->pe, that contains an address of the end
Typo: s/the LexState/LexState/
> of bytecode chunk in the memory, will wrap around and become smaller
Typo: s/of bytecode/of the bytecode/
> than address in LexState->p, that contains an address of the beginning
Typo: s/than address/than the address/
> of bytecode chunk in the memory. In bcread_fill() called by
> bcread_want(), memcpy() is called with a very large size and causes bus
> error on x86 and segmentation fault on ARM android.
Typo: s/android/Android/
> 
> The problem cannot be reproduced on platforms supported by Tarantool
> (ARM64, x86_64), so test doesn't reproduce a problem without a patch and
> tests patch partially.
Typo: s/tests the patch/

Well, I've tried to run that test on x32 machine, and nothing happened.
I think we should backport this patch without tests then, since this
test seems irrelevant to the problem. What do you think?

Also, it is kinda strange, that you are talking about a test in a patch
without any tests. You need to either mention that the test is present in
the next commit, or move that test here.
> 
> Sergey Bronnikov:
> * added the description
> ---
>  src/lj_bcread.c | 10 +++++-----
>  src/lj_lex.c    |  6 ++++++
>  src/lj_lex.h    |  1 +
>  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/src/lj_bcread.c b/src/lj_bcread.c
> index f6c7ad25..315ad4d8 100644
> --- a/src/lj_bcread.c
> +++ b/src/lj_bcread.c
> @@ -79,6 +79,7 @@ static LJ_NOINLINE void bcread_fill(LexState *ls, MSize len, int need)
>        ls->c = -1;  /* Only bad if we get called again. */
>        break;
>      }
> +    if (sz >= LJ_MAX_BUF - n) lj_err_mem(ls->L);
>      if (n) {  /* Append to buffer. */
>        n += (MSize)sz;
>        p = lj_buf_need(&ls->sb, n < len ? len : n);
> @@ -90,20 +91,20 @@ static LJ_NOINLINE void bcread_fill(LexState *ls, MSize len, int need)
>        ls->p = buf;
>        ls->pe = buf + sz;
>      }
> -  } while (ls->p + len > ls->pe);
> +  } while ((MSize)(ls->pe - ls->p) < len);
>  }
>  
>  /* Need a certain number of bytes. */
>  static LJ_AINLINE void bcread_need(LexState *ls, MSize len)
>  {
> -  if (LJ_UNLIKELY(ls->p + len > ls->pe))
> +  if (LJ_UNLIKELY((MSize)(ls->pe - ls->p) < len))
>      bcread_fill(ls, len, 1);
>  }
>  
>  /* Want to read up to a certain number of bytes, but may need less. */
>  static LJ_AINLINE void bcread_want(LexState *ls, MSize len)
>  {
> -  if (LJ_UNLIKELY(ls->p + len > ls->pe))
> +  if (LJ_UNLIKELY((MSize)(ls->pe - ls->p) < len))
>      bcread_fill(ls, len, 0);
>  }
>  
> @@ -463,8 +464,7 @@ GCproto *lj_bcread(LexState *ls)
>      setprotoV(L, L->top, pt);
>      incr_top(L);
>    }
> -  if ((int32_t)(2*(uint32_t)(ls->pe - ls->p)) > 0 ||
> -      L->top-1 != bcread_oldtop(L, ls))
> +  if ((ls->pe != ls->p && !ls->endmark) || L->top-1 != bcread_oldtop(L, ls))
>      bcread_error(ls, LJ_ERR_BCBAD);
>    /* Pop off last prototype. */
>    L->top--;
> diff --git a/src/lj_lex.c b/src/lj_lex.c
> index 52856912..82e4ba6f 100644
> --- a/src/lj_lex.c
> +++ b/src/lj_lex.c
> @@ -48,6 +48,11 @@ static LJ_NOINLINE LexChar lex_more(LexState *ls)
>    size_t sz;
>    const char *p = ls->rfunc(ls->L, ls->rdata, &sz);
>    if (p == NULL || sz == 0) return LEX_EOF;
> +  if (sz >= LJ_MAX_BUF) {
> +    if (sz != ~(size_t)0) lj_err_mem(ls->L);
> +    sz = ~(uintptr_t)0 - (uintptr_t)p;
> +    ls->endmark = 1;
> +  }
>    ls->pe = p + sz;
>    ls->p = p + 1;
>    return (LexChar)(uint8_t)p[0];
> @@ -406,6 +411,7 @@ int lj_lex_setup(lua_State *L, LexState *ls)
>    ls->lookahead = TK_eof;  /* No look-ahead token. */
>    ls->linenumber = 1;
>    ls->lastline = 1;
> +  ls->endmark = 0;
>    lex_next(ls);  /* Read-ahead first char. */
>    if (ls->c == 0xef && ls->p + 2 <= ls->pe && (uint8_t)ls->p[0] == 0xbb &&
>        (uint8_t)ls->p[1] == 0xbf) {  /* Skip UTF-8 BOM (if buffered). */
> diff --git a/src/lj_lex.h b/src/lj_lex.h
> index 33fa8657..38d28533 100644
> --- a/src/lj_lex.h
> +++ b/src/lj_lex.h
> @@ -73,6 +73,7 @@ typedef struct LexState {
>    BCInsLine *bcstack;	/* Stack for bytecode instructions/line numbers. */
>    MSize sizebcstack;	/* Size of bytecode stack. */
>    uint32_t level;	/* Syntactical nesting level. */
> +  int endmark;		/* Trust bytecode end marker, even if not at EOF. */
>  } LexState;
>  
>  LJ_FUNC int lj_lex_setup(lua_State *L, LexState *ls);
> -- 
> 2.34.1

Re: [Tarantool-patches] [PATCH 2/2] Followup fix for embedded bytecode loader.

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.

On Tue, Jul 25, 2023 at 07:37:01PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
> From: sergeyb@tarantool.org
> 
> (cherry-picked from commit e49863eda13d095b1a78fd4ca0fd3a6a9a17d782)
The description is missing. As we've already discussed offline, it is generally
not a good practice to describe a commit in the previous commit. So it would be
great if you moved the relevant part of the description here instead.

> 
> Sergey Bronnikov:
>   * added the partial test for this and a previous patch
> ---
>  src/lj_lex.c                                  |   1 +
>  test/tarantool-c-tests/lj-549-lua_load.test.c | 146 ++++++++++++++++++
>  2 files changed, 147 insertions(+)
>  create mode 100644 test/tarantool-c-tests/lj-549-lua_load.test.c
> 
> diff --git a/src/lj_lex.c b/src/lj_lex.c
> index 82e4ba6f..161d862e 100644
> --- a/src/lj_lex.c
> +++ b/src/lj_lex.c
> @@ -51,6 +51,7 @@ static LJ_NOINLINE LexChar lex_more(LexState *ls)
>    if (sz >= LJ_MAX_BUF) {
>      if (sz != ~(size_t)0) lj_err_mem(ls->L);
>      sz = ~(uintptr_t)0 - (uintptr_t)p;
> +    if (sz >= LJ_MAX_BUF) sz = LJ_MAX_BUF-1;
>      ls->endmark = 1;
>    }
>    ls->pe = p + sz;
> diff --git a/test/tarantool-c-tests/lj-549-lua_load.test.c b/test/tarantool-c-tests/lj-549-lua_load.test.c
> new file mode 100644
> index 00000000..4fb144cf
> --- /dev/null
> +++ b/test/tarantool-c-tests/lj-549-lua_load.test.c

As I have already mentioned in the previous patch, it passes with no problems
whatsoever before the patch on x32 machine, so I am not sure we need to add it.

> @@ -0,0 +1,146 @@
> +#include <assert.h>
> +#include <stdint.h>
> +#include <stddef.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include <lua.h>
> +#include <lualib.h>
> +#include <lauxlib.h>
> +
> +#include "test.h"
> +#include "utils.h"
> +
> +/* Need for skipcond. */
> +#include "lj_arch.h"
> +
> +/* Defined in lj_def.h. */
> +#define LJ_MAX_MEM32    0x7fffff00      /* Max. 32 bit memory allocation. */
> +#define LJ_MAX_BUF      LJ_MAX_MEM32    /* Max. buffer length. */
> +
> +#define UNUSED(x) ((void)(x))
> +
> +/**
> + * Array with bytecode was generated using commands below:
> + *
> + * cat << EOF > a.lua
> + * local a = 1
> + * EOF
> + * luajit -b a.lua a.h
> + */
> +#define luaJIT_BC_sample_SIZE 22
> +static const unsigned char luaJIT_BC_sample[] = {
> +27,76,74,2,2,15,2,0,1,0,0,0,2,41,0,1,0,75,0,1,0,0
Missing spaces.
> +};
> +
> +/**
> + * Function generates a huge chunk with "bytecode" with a size bigger than
Typo: s/with "bytecode"/of "bytecode"/
> + * LJ_MAX_BUF. Generated chunk must enable endmark in a Lex state.
> + */
> +static const char *
> +bc_reader_with_endmark(lua_State *L, void *data, size_t *size)
> +{
> +	UNUSED(data);
> +	static char *bc_chunk = NULL;
> +	free(bc_chunk);
> +
> +	int bc_chunk_size = (size_t)0;
We ussually follow the C89 standard, which mentions that variables must be
declared in the beginning of the block. Here and below.
> +	bc_chunk = malloc(bc_chunk_size);
> +	assert(bc_chunk != NULL);
> +
> +	/**
> +	 * Put a chunk with a valid bytecode to the beginning of allocated region.
Typo: s/to the beginning/at the beginning/
Typo: s/of allocated/of the allocated/
> +	 * lua_load automatically detects whether the chunk is text or binary,
Typo: s/lua_load/`lua_load`/ Feel free to ignore, though.
> +	 * and loads it accordingly. We need a trace for bytecode input,
> +	 * so it is necessary to deceive a check in lj_lex_setup, that makes a
> +	 * sanity check and detects whether input is bytecode or text by first char.
Typo: s/by first/by the first/
> +	 * Strictly speaking it is enough to put LUA_SIGNATURE[0] as a first
Typo: s/Strictly speaking/Strictly speaking,/
Typo: s/a first/the first/
> +	 * symbol in produced chunk.
Typo: s/in produced/in the produced/
> +	 */
> +	memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE);
> +
> +	*size = bc_chunk_size;
> +
> +	return bc_chunk;
> +}
> +
> +static int bc_loader_with_endmark(void *test_state)
> +{
> +	lua_State *L = test_state;
> +	void *ud = NULL;
> +	int res = lua_load(L, bc_reader_with_endmark, ud, "endmark");
> +
> +	/* Make sure we passed condition with lj_err_mem in a function lex_more. */
Typo: s/passed condition/passed the condition/
Typo: s/in a function/in the function/
> +	assert_true(res != LUA_ERRMEM);
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +enum bc_emission_state {
> +	EMIT_BC,
> +	EMIT_EOF,
> +};
> +
> +typedef struct {
> +	enum bc_emission_state state;
> +} dt;
> +
> +/**
> + * Function returns bytecode chunk on the first call and NULL and size equals
Typo: s/returns bytecode/returns a bytecode/
Typo: s/equals/equal/
> + * to zero on the second call. Triggers END_OF_STREAM flag in a function
Typo: s/Triggers/Triggers the/
Typo: s/in a function/in the function/
> + * lex_more.
> + */
> +static const char *
> +bc_reader_with_eof(lua_State *L, void *data, size_t *size)
> +{
> +	UNUSED(data);
> +	UNUSED(L);
> +	dt *test_data = (dt *)data;
> +	if (test_data->state == EMIT_EOF) {
> +		*size = 0;
> +		return NULL;
> +	}
> +
> +	char *bc_chunk = malloc(luaJIT_BC_sample_SIZE);
> +	memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE);
> +	*size = luaJIT_BC_sample_SIZE;
> +
> +	return bc_chunk;
> +}
> +
> +static int bc_loader_with_eof(void *test_state)
> +{
> +	lua_State *L = test_state;
> +	dt test_data = {0};
> +	test_data.state = EMIT_BC;
> +	int res = lua_load(L, bc_reader_with_eof, &test_data, "eof");
> +	assert_true(res = LUA_ERRSYNTAX);
> +	if (res == LUA_OK) {
> +		lua_pcall(L, 0, 0, 0);
> +	}
> +
> +	return TEST_EXIT_SUCCESS;
> +}
> +
> +int main(void)
> +{
> +	if (LJ_GC64 || !LUAJIT_ARCH_X64 || !LJ_TARGET_LINUX)
> +		/**
> +		 * lua_load source code is common on all platforms,
> +		 * when bytecode is not portable.
> +		 * So test runs on Linux/x86_64 only and skipped on other
Typo: s/So test/So the test/
Typo: s/and skipped/and is skipped/
> +		 * platforms.
> +		 */
> +		return skip_all("Enabled on Linux/x86_64 with disabled GC64");
We prefer to run tests like that on all paltforms, just to be sure. We usually
exclude tests from platforms only if those are completely irrelevant to the test,
or there is some kind of major issue.
> +
> +	lua_State *L = utils_lua_init();
> +	const struct test_unit tgroup[] = {
> +		test_unit_def(bc_loader_with_endmark),
> +		test_unit_def(bc_loader_with_eof)
> +	};
> +
> +	const int test_result = test_run_group(tgroup, L);
> +	utils_lua_close(L);
> +	return test_result;
> +}
> -- 
> 2.34.1
> 

Re: [Tarantool-patches] [PATCH 2/2] Followup fix for embedded bytecode loader.

[Sergey Bronnikov via Tarantool-patches]

Hi, Max!

thanks for your comments. See my answers.

On 7/31/23 15:20, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On Tue, Jul 25, 2023 at 07:37:01PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
>> From: sergeyb@tarantool.org
>>
>> (cherry-picked from commit e49863eda13d095b1a78fd4ca0fd3a6a9a17d782)
> The description is missing. As we've already discussed offline, it is generally
> not a good practice to describe a commit in the previous commit. So it would be
> great if you moved the relevant part of the description here instead.
Description added to the second commit message as well.
>
>> Sergey Bronnikov:
>>    * added the partial test for this and a previous patch
>> ---
>>   src/lj_lex.c                                  |   1 +
>>   test/tarantool-c-tests/lj-549-lua_load.test.c | 146 ++++++++++++++++++
>>   2 files changed, 147 insertions(+)
>>   create mode 100644 test/tarantool-c-tests/lj-549-lua_load.test.c
>>
>> diff --git a/src/lj_lex.c b/src/lj_lex.c
>> index 82e4ba6f..161d862e 100644
>> --- a/src/lj_lex.c
>> +++ b/src/lj_lex.c
>> @@ -51,6 +51,7 @@ static LJ_NOINLINE LexChar lex_more(LexState *ls)
>>     if (sz >= LJ_MAX_BUF) {
>>       if (sz != ~(size_t)0) lj_err_mem(ls->L);
>>       sz = ~(uintptr_t)0 - (uintptr_t)p;
>> +    if (sz >= LJ_MAX_BUF) sz = LJ_MAX_BUF-1;
>>       ls->endmark = 1;
>>     }
>>     ls->pe = p + sz;
>> diff --git a/test/tarantool-c-tests/lj-549-lua_load.test.c b/test/tarantool-c-tests/lj-549-lua_load.test.c
>> new file mode 100644
>> index 00000000..4fb144cf
>> --- /dev/null
>> +++ b/test/tarantool-c-tests/lj-549-lua_load.test.c
> As I have already mentioned in the previous patch, it passes with no problems
> whatsoever before the patch on x32 machine, so I am not sure we need to add it.

Test for a public function is better than nothing. I would left it, but 
we can also wait

for opinion of the second reviewer and then decide.

>
>> @@ -0,0 +1,146 @@
>> +#include <assert.h>
>> +#include <stdint.h>
>> +#include <stddef.h>
>> +#include <string.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +
>> +#include <lua.h>
>> +#include <lualib.h>
>> +#include <lauxlib.h>
>> +
>> +#include "test.h"
>> +#include "utils.h"
>> +
>> +/* Need for skipcond. */
>> +#include "lj_arch.h"
>> +
>> +/* Defined in lj_def.h. */
>> +#define LJ_MAX_MEM32    0x7fffff00      /* Max. 32 bit memory allocation. */
>> +#define LJ_MAX_BUF      LJ_MAX_MEM32    /* Max. buffer length. */
>> +
>> +#define UNUSED(x) ((void)(x))
>> +
>> +/**
>> + * Array with bytecode was generated using commands below:
>> + *
>> + * cat << EOF > a.lua
>> + * local a = 1
>> + * EOF
>> + * luajit -b a.lua a.h
>> + */
>> +#define luaJIT_BC_sample_SIZE 22
>> +static const unsigned char luaJIT_BC_sample[] = {
>> +27,76,74,2,2,15,2,0,1,0,0,0,2,41,0,1,0,75,0,1,0,0
> Missing spaces.
Fixed.
>> +};
>> +
>> +/**
>> + * Function generates a huge chunk with "bytecode" with a size bigger than
> Typo: s/with "bytecode"/of "bytecode"/
Fixed.
>> + * LJ_MAX_BUF. Generated chunk must enable endmark in a Lex state.
>> + */
>> +static const char *
>> +bc_reader_with_endmark(lua_State *L, void *data, size_t *size)
>> +{
>> +	UNUSED(data);
>> +	static char *bc_chunk = NULL;
>> +	free(bc_chunk);
>> +
>> +	int bc_chunk_size = (size_t)0;
> We ussually follow the C89 standard, which mentions that variables must be
> declared in the beginning of the block. Here and below.
Fixed.
>> +	bc_chunk = malloc(bc_chunk_size);
>> +	assert(bc_chunk != NULL);
>> +
>> +	/**
>> +	 * Put a chunk with a valid bytecode to the beginning of allocated region.
> Typo: s/to the beginning/at the beginning/
> Typo: s/of allocated/of the allocated/
Fixed.
>> +	 * lua_load automatically detects whether the chunk is text or binary,
> Typo: s/lua_load/`lua_load`/ Feel free to ignore, though.
Fixed.
>> +	 * and loads it accordingly. We need a trace for bytecode input,
>> +	 * so it is necessary to deceive a check in lj_lex_setup, that makes a
>> +	 * sanity check and detects whether input is bytecode or text by first char.
> Typo: s/by first/by the first/
Fixed.
>> +	 * Strictly speaking it is enough to put LUA_SIGNATURE[0] as a first
> Typo: s/Strictly speaking/Strictly speaking,/
> Typo: s/a first/the first/
Fixed.
>> +	 * symbol in produced chunk.
> Typo: s/in produced/in the produced/
Fixed.
>> +	 */
>> +	memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE);
>> +
>> +	*size = bc_chunk_size;
>> +
>> +	return bc_chunk;
>> +}
>> +
>> +static int bc_loader_with_endmark(void *test_state)
>> +{
>> +	lua_State *L = test_state;
>> +	void *ud = NULL;
>> +	int res = lua_load(L, bc_reader_with_endmark, ud, "endmark");
>> +
>> +	/* Make sure we passed condition with lj_err_mem in a function lex_more. */
> Typo: s/passed condition/passed the condition/
> Typo: s/in a function/in the function/
Fixed.
>> +	assert_true(res != LUA_ERRMEM);
>> +
>> +	return TEST_EXIT_SUCCESS;
>> +}
>> +
>> +enum bc_emission_state {
>> +	EMIT_BC,
>> +	EMIT_EOF,
>> +};
>> +
>> +typedef struct {
>> +	enum bc_emission_state state;
>> +} dt;
>> +
>> +/**
>> + * Function returns bytecode chunk on the first call and NULL and size equals
> Typo: s/returns bytecode/returns a bytecode/
> Typo: s/equals/equal/
Fixed.
>> + * to zero on the second call. Triggers END_OF_STREAM flag in a function
> Typo: s/Triggers/Triggers the/
> Typo: s/in a function/in the function/
Fixed.
>> + * lex_more.
>> + */
>> +static const char *
>> +bc_reader_with_eof(lua_State *L, void *data, size_t *size)
>> +{
>> +	UNUSED(data);
>> +	UNUSED(L);
>> +	dt *test_data = (dt *)data;
>> +	if (test_data->state == EMIT_EOF) {
>> +		*size = 0;
>> +		return NULL;
>> +	}
>> +
>> +	char *bc_chunk = malloc(luaJIT_BC_sample_SIZE);
>> +	memcpy(bc_chunk, luaJIT_BC_sample, luaJIT_BC_sample_SIZE);
>> +	*size = luaJIT_BC_sample_SIZE;
>> +
>> +	return bc_chunk;
>> +}
>> +
>> +static int bc_loader_with_eof(void *test_state)
>> +{
>> +	lua_State *L = test_state;
>> +	dt test_data = {0};
>> +	test_data.state = EMIT_BC;
>> +	int res = lua_load(L, bc_reader_with_eof, &test_data, "eof");
>> +	assert_true(res = LUA_ERRSYNTAX);
>> +	if (res == LUA_OK) {
>> +		lua_pcall(L, 0, 0, 0);
>> +	}
>> +
>> +	return TEST_EXIT_SUCCESS;
>> +}
>> +
>> +int main(void)
>> +{
>> +	if (LJ_GC64 || !LUAJIT_ARCH_X64 || !LJ_TARGET_LINUX)
>> +		/**
>> +		 * lua_load source code is common on all platforms,
>> +		 * when bytecode is not portable.
>> +		 * So test runs on Linux/x86_64 only and skipped on other
> Typo: s/So test/So the test/
> Typo: s/and skipped/and is skipped/
Fixed.
>> +		 * platforms.
>> +		 */
>> +		return skip_all("Enabled on Linux/x86_64 with disabled GC64");
> We prefer to run tests like that on all paltforms, just to be sure. We usually
> exclude tests from platforms only if those are completely irrelevant to the test,
> or there is some kind of major issue.

Yes, for this test the rule was ignored because bytecode is not portable

and following the rule will make maintenance difficult. Therefore 
supported platforms limited by x86_64 only.

>> +
>> +	lua_State *L = utils_lua_init();
>> +	const struct test_unit tgroup[] = {
>> +		test_unit_def(bc_loader_with_endmark),
>> +		test_unit_def(bc_loader_with_eof)
>> +	};
>> +
>> +	const int test_result = test_run_group(tgroup, L);
>> +	utils_lua_close(L);
>> +	return test_result;
>> +}
>> -- 
>> 2.34.1
>>

Re: [Tarantool-patches] [PATCH 1/2] Fix embedded bytecode loader.

[Sergey Bronnikov via Tarantool-patches]

Hi, Max


thanks for review! see my comments


On 7/31/23 15:01, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
>
> On Tue, Jul 25, 2023 at 07:36:24PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
>> From: sergeyb@tarantool.org
>>
>> (cherry-picked from commit 820339960123dc78a7ce03edf53fcf4fdae0e55d)
>>
>> Original problem is specific for x32 and is as follows: when a chunk
> Typo: s/Original/The original/
> Typo: s/for x32/to x32/
Fixed.
>> with bytecode library is loaded into memory, and the address is higher
> Typo: s/with bytecode/with a bytecode/
Fixed.
>> than 0x80000100, the LexState->pe, that contains an address of the end
> Typo: s/the LexState/LexState/
Fixed.
>> of bytecode chunk in the memory, will wrap around and become smaller
> Typo: s/of bytecode/of the bytecode/
Fixed.
>> than address in LexState->p, that contains an address of the beginning
> Typo: s/than address/than the address/
Fixed.
>> of bytecode chunk in the memory. In bcread_fill() called by
>> bcread_want(), memcpy() is called with a very large size and causes bus
>> error on x86 and segmentation fault on ARM android.
> Typo: s/android/Android/
Fixed.
>> The problem cannot be reproduced on platforms supported by Tarantool
>> (ARM64, x86_64), so test doesn't reproduce a problem without a patch and
>> tests patch partially.
> Typo: s/tests the patch/
Fixed.
>
> Well, I've tried to run that test on x32 machine, and nothing happened.
> I think we should backport this patch without tests then, since this
> test seems irrelevant to the problem. What do you think?
It is relevant, because covers a part of the patch.
>
> Also, it is kinda strange, that you are talking about a test in a patch
> without any tests. You need to either mention that the test is present in
> the next commit, or move that test here.
I'll update commit message, thanks.
<snipped>

Re: [Tarantool-patches] [PATCH 1/2] Fix embedded bytecode loader.

[Sergey Kaplun via Tarantool-patches]

Hi, Max, Sergey!
The patch is LGTM, after adding the test case (without additional CI
job, if you don't want), see rational below.

On 31.07.23, Maxim Kokryashkin wrote:
> Hi, Sergey!

<snipped>

> > tests patch partially.
> Typo: s/tests the patch/
> 
> Well, I've tried to run that test on x32 machine, and nothing happened.
> I think we should backport this patch without tests then, since this
> test seems irrelevant to the problem. What do you think?
> 
> Also, it is kinda strange, that you are talking about a test in a patch
> without any tests. You need to either mention that the test is present in
> the next commit, or move that test here.

The problem is perfectly repoduced for me if use instructions from [1].

I suppose that we should add this test, because it:
1) Shows the problem for the actual build that can be done by the user.
2) Still tests LuaJIT loader.

> > 
> > Sergey Bronnikov:
> > * added the description
> > ---
> >  src/lj_bcread.c | 10 +++++-----
> >  src/lj_lex.c    |  6 ++++++
> >  src/lj_lex.h    |  1 +
> >  3 files changed, 12 insertions(+), 5 deletions(-)

<snipped>

> > -- 
> > 2.34.1

[1]: https://github.com/LuaJIT/LuaJIT/issues/549

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 2/2] Followup fix for embedded bytecode loader.

[Sergey Bronnikov via Tarantool-patches]

Hi, Max


On 8/1/23 12:53, Sergey Bronnikov via Tarantool-patches wrote:


<snipped>

>>> +         * platforms.
>>> +         */
>>> +        return skip_all("Enabled on Linux/x86_64 with disabled GC64");
>> We prefer to run tests like that on all paltforms, just to be sure. 
>> We usually
>> exclude tests from platforms only if those are completely irrelevant 
>> to the test,
>> or there is some kind of major issue.
>
> Yes, for this test the rule was ignored because bytecode is not portable
>
> and following the rule will make maintenance difficult. Therefore 
> supported platforms limited by x86_64 only.
>
Test has been updated: bytecode is not needed and now test runs on all 
platforms.

See patch below:


diff --git a/test/tarantool-c-tests/lj-549-lua_load.test.c 
b/test/tarantool-c-tests/lj-549-lua_load.test.c
index fd1e20ed..669b6b27 100644
--- a/test/tarantool-c-tests/lj-549-lua_load.test.c
+++ b/test/tarantool-c-tests/lj-549-lua_load.test.c
@@ -19,20 +19,11 @@
  #define LJ_MAX_MEM32    0x7fffff00      /* Max. 32 bit memory 
allocation. */
  #define LJ_MAX_BUF      LJ_MAX_MEM32    /* Max. buffer length. */

-#define UNUSED(x) ((void)(x))
+/* Defined in lua.h. */
+/* mark for precompiled code (`<esc>Lua') */
+#define    LUA_SIGNATURE    "\033Lua"

-/**
- * Array with bytecode was generated using commands below:
- *
- * cat << EOF > a.lua
- * local a = 1
- * EOF
- * luajit -b a.lua a.h
- */
-#define luaJIT_BC_x86_64_sample_SIZE 22
-static const unsigned char luaJIT_BC_x86_64_sample[] = {
-    27, 76, 74, 2, 2, 15, 2, 0, 1, 0, 0, 0, 2, 41, 0, 1, 0, 75, 0, 1, 0, 0
-};
+#define UNUSED(x) ((void)(x))

  /**
   * Function generates a huge chunk of "bytecode" with a size bigger than
@@ -50,15 +41,14 @@ bc_reader_with_endmark(lua_State *L, void *data, 
size_t *size)
      assert(bc_chunk != NULL);

      /**
-     * Put a chunk a valid bytecode at the beginning of the allocated
-     * region. `lua_load` automatically detects whether the chunk is text
-     * or binary, and loads it accordingly. We need a trace for bytecode
-     * input, so it is necessary to deceive a check in lj_lex_setup, that
+     * `lua_load` automatically detects whether the chunk is text or 
binary,
+     * and loads it accordingly. We need a trace for bytecode input,
+     * so it is necessary to deceive a check in lj_lex_setup, that
       * makes a sanity check and detects whether input is bytecode or text
-     * by the first char. Strictly speaking, it is enough to put
-     * LUA_SIGNATURE[0] as the first symbol in the produced chunk.
+     * by the first char. Put LUA_SIGNATURE[0] at the beginning of the
+     * allocated region.
       */
-    memcpy(bc_chunk, luaJIT_BC_x86_64_sample, 
luaJIT_BC_x86_64_sample_SIZE);
+    bc_chunk[0] = LUA_SIGNATURE[0];

      *size = bc_chunk_size;

@@ -105,9 +95,10 @@ bc_reader_with_eof(lua_State *L, void *data, size_t 
*size)
          return NULL;
      }

-    char *bc_chunk = malloc(luaJIT_BC_x86_64_sample_SIZE);
-    memcpy(bc_chunk, luaJIT_BC_x86_64_sample, 
luaJIT_BC_x86_64_sample_SIZE);
-    *size = luaJIT_BC_x86_64_sample_SIZE;
+    size_t sz = 10;
+    char *bc_chunk = malloc(sz);
+    bc_chunk[0] = LUA_SIGNATURE[0];
+    *size = sz;

      return bc_chunk;
  }
@@ -128,15 +119,6 @@ static int bc_loader_with_eof(void *test_state)

  int main(void)
  {
-    if (LJ_GC64 || !LUAJIT_ARCH_X64 || !LJ_TARGET_LINUX)
-        /**
-         * `lua_load` source code is common on all platforms,
-         * when bytecode is not portable.
-         * So the test runs on Linux/x86_64 only and is skipped on other
-         * platforms.
-         */
-        return skip_all("Enabled on Linux/x86_64 with disabled GC64");
-
      lua_State *L = utils_lua_init();
      const struct test_unit tgroup[] = {
          test_unit_def(bc_loader_with_endmark),


>>> +
>>> +    lua_State *L = utils_lua_init();
>>> +    const struct test_unit tgroup[] = {
>>> +        test_unit_def(bc_loader_with_endmark),
>>> +        test_unit_def(bc_loader_with_eof)
>>> +    };
>>> +
>>> +    const int test_result = test_run_group(tgroup, L);
>>> +    utils_lua_close(L);
>>> +    return test_result;
>>> +}
>>> -- 
>>> 2.34.1
>>>

Re: [Tarantool-patches] [PATCH 1/2] Fix embedded bytecode loader.

[Maxim Kokryashkin via Tarantool-patches]

Hi!
Ok, makes sense. LGTM, then, but I really want Sergey to include the
`smoke test` term somewhere in the commit description, so it is obvious,
that we are testing the basic functionality.

On Sun, Aug 06, 2023 at 02:09:27PM +0300, Sergey Kaplun wrote:
> Hi, Max, Sergey!
> The patch is LGTM, after adding the test case (without additional CI
> job, if you don't want), see rational below.
> 
> On 31.07.23, Maxim Kokryashkin wrote:
> > Hi, Sergey!
> 
> <snipped>
> 
> > > tests patch partially.
> > Typo: s/tests the patch/
> > 
> > Well, I've tried to run that test on x32 machine, and nothing happened.
> > I think we should backport this patch without tests then, since this
> > test seems irrelevant to the problem. What do you think?
> > 
> > Also, it is kinda strange, that you are talking about a test in a patch
> > without any tests. You need to either mention that the test is present in
> > the next commit, or move that test here.
> 
> The problem is perfectly repoduced for me if use instructions from [1].
> 
> I suppose that we should add this test, because it:
> 1) Shows the problem for the actual build that can be done by the user.
> 2) Still tests LuaJIT loader.
> 
> > > 
> > > Sergey Bronnikov:
> > > * added the description
> > > ---
> > >  src/lj_bcread.c | 10 +++++-----
> > >  src/lj_lex.c    |  6 ++++++
> > >  src/lj_lex.h    |  1 +
> > >  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> <snipped>
> 
> > > -- 
> > > 2.34.1
> 
> [1]: https://github.com/LuaJIT/LuaJIT/issues/549
> 
> -- 
> Best regards,
> Sergey Kaplun