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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Max

On 9/1/23 13:05, Maxim Kokryashkin via Tarantool-patches wrote:
> On Thu, Aug 31, 2023 at 02:32:14PM +0300, Sergey Bronnikov via Tarantool-patches wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
<snipped>
>> +
>> +/**
>> + * Function generates a huge chunk of "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);
>> +	int bc_chunk_size = (size_t)0;
>> +	static char *bc_chunk = NULL;
>> +	free(bc_chunk);
> What's the point of free here? Why the buffer is static?

Because callee (aka Reader) is responsible for buffer, reader 
initializes it once

and then reuse.

 > The block must exist until the reader function is called again.

 > To signal the end of the chunk, the reader must return NULL or set 
size to zero.

 > The reader function may return pieces of any size greater than zero.


1. http://www.lua.org/manual/5.1/manual.html#lua_Reader


>> +
>> +	bc_chunk = malloc(bc_chunk_size);
> Malloc of zero size doesn't seem to be the thing you wanted to do.

Right. Updated:


@@ -33,26 +33,9 @@ static const char *
  bc_reader_with_endmark(lua_State *L, void *data, size_t *size)
  {
      UNUSED(data);
-    int bc_chunk_size = (size_t)0;
-    static char *bc_chunk = NULL;
-    free(bc_chunk);
-
-    bc_chunk = malloc(bc_chunk_size);
-    assert(bc_chunk != NULL);
-
-    /**
-     * `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. Put LUA_SIGNATURE[0] at the beginning of the
-     * allocated region.
-     */
-    bc_chunk[0] = LUA_SIGNATURE[0];
-
-    *size = bc_chunk_size;
+    *size = ~(size_t)0;

-    return bc_chunk;
+    return NULL;
  }

  static int bc_loader_with_endmark(void *test_state)

>> +	assert(bc_chunk != NULL);
>> +
>> +	/**
>> +	 * `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. Put LUA_SIGNATURE[0] at the beginning of the
>> +	 * allocated region.
>> +	 */
>> +	bc_chunk[0] = LUA_SIGNATURE[0];
>> +
>> +	*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 the condition with lj_err_mem in the 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 a bytecode chunk on the first call and NULL and size equal
>> + * to zero on the second call. Triggers the END_OF_STREAM flag 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) {
> This section is unreachable, isn't it?

Right, fixed it.


>> +		*size = 0;
>> +		return NULL;
>> +	}
>> +
>> +	static char *bc_chunk = NULL;
>> +	free(bc_chunk);
> Ditto.
>> +
>> +	size_t sz = 10;
> Is there any reason for it to be exactly 10? Drop a comment.

Set it to 2 and dropped a comment.


@@ -98,10 +81,23 @@ bc_reader_with_eof(lua_State *L, void *data, size_t 
*size)
      static char *bc_chunk = NULL;
      free(bc_chunk);

-    size_t sz = 10;
+    /**
+     * Minimal size of a buffer with bytecode:
+     * signiture (1 byte) and a bytecode itself (1 byte).
+     */
+    size_t sz = 2;
      bc_chunk = malloc(sz);
+    /**
+     * `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. Put `LUA_SIGNATURE[0]` at the beginning of the
+     * allocated region.
+     */
      bc_chunk[0] = LUA_SIGNATURE[0];
      *size = sz;
+    test_data->state = EMIT_EOF;

      return bc_chunk;
  }


<snipped>