Re: [Tarantool-patches] [PATCH v2 corrected] Increase limits for debug.traceback

[Igor Munkin <imun@tarantool.org>]

Sergey,

Thanks, the patch is nice, except the changes around LUA_IDSIZE_ERR I
was going to reply (but you've sent the second version in a jiffy).

Furthermore I left a few polishing-aimed comments below, please consider
them too.

On 22.01.20, Sergey Voinov wrote:
> Currently debug.traceback cuts out path prefixes
> and removes intermediate frames. This makes it harder
> to debug and navigate to files right from terminal.
> 
> This change increases traceback limits to 25 stack frames
> and maximum source file name length to 128 characters.

AFAICS the change just introduce the compile-time constants to be
tuned for Tarantool built. The defaults are left the same, so please
adjust the commit message.

> 
> There are also changes in main Tarantool trunk in
> cmake/luajit.cmake (see corresponding commit for it).

You can move this remark to the changelog below, since the patch can be
applied w/o the corresponding changes in Tarantool repo.

> 
> Closes: #4581

Please, don't use ':' in gh tags. It just doesn't respect our commit
message style.

> ---
>  Changes in v2:
>  - fixed code and test in accordance with review
> 
>  src/lj_debug.c                         |  8 +--
>  src/luaconf.h                          | 14 ++++-
>  test/tarantool-4581-traceback.test.lua | 79 ++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+), 6 deletions(-)
>  create mode 100755 test/tarantool-4581-traceback.test.lua
> 
> diff --git a/src/lj_debug.c b/src/lj_debug.c
> index 73bd196..e65eb05 100644
> --- a/src/lj_debug.c
> +++ b/src/lj_debug.c
> @@ -332,11 +332,11 @@ void lj_debug_shortname(char *out, GCstr *str, BCLine line)
>      strcpy(out, src);
>    } else {  /* Output [string "string"] or [builtin:name]. */
>      size_t len;  /* Length, up to first control char. */
> -    for (len = 0; len < LUA_IDSIZE-12; len++)
> +    for (len = 0; len < LUA_IDSIZE_ERR-12; len++)
>        if (((const unsigned char *)src)[len] < ' ') break;
>      strcpy(out, line == ~(BCLine)0 ? "[builtin:" : "[string \""); out += 9;
>      if (src[len] != '\0') {  /* Must truncate? */
> -      if (len > LUA_IDSIZE-15) len = LUA_IDSIZE-15;
> +      if (len > LUA_IDSIZE_ERR-15) len = LUA_IDSIZE_ERR-15;
>        strncpy(out, src, len); out += len;
>        strcpy(out, "..."); out += 3;
>      } else {
> @@ -651,8 +651,8 @@ void lj_debug_dumpstack(lua_State *L, SBuf *sb, const char *fmt, int depth)
>  #endif
>  
>  /* Number of frames for the leading and trailing part of a traceback. */
> -#define TRACEBACK_LEVELS1	12
> -#define TRACEBACK_LEVELS2	10
> +#define TRACEBACK_LEVELS1	LUA_TRACEBACK_LEVELS1
> +#define TRACEBACK_LEVELS2	LUA_TRACEBACK_LEVELS2
>  
>  LUALIB_API void luaL_traceback (lua_State *L, lua_State *L1, const char *msg,
>  				int level)
> diff --git a/src/luaconf.h b/src/luaconf.h
> index 60cb928..69992b3 100644
> --- a/src/luaconf.h
> +++ b/src/luaconf.h
> @@ -109,9 +109,19 @@
>  #define LUA_MAXINPUT	512	/* Max. input line length. */
>  #endif
>  
> +#ifndef LUA_TRACEBACK_LEVELS1
> +# define LUA_TRACEBACK_LEVELS1	12
> +#endif
> +#ifndef LUA_TRACEBACK_LEVELS2
> +# define LUA_TRACEBACK_LEVELS2	10
> +#endif
> +
>  /* Note: changing the following defines breaks the Lua 5.1 ABI. */
> -#define LUA_INTEGER	ptrdiff_t
> -#define LUA_IDSIZE	60	/* Size of lua_Debug.short_src. */
> +#define LUA_INTEGER		ptrdiff_t
> +#ifndef LUA_IDSIZE
> +# define LUA_IDSIZE		128 /* Size of lua_Debug.short_src. */
> +#endif

Minor: I guess the wrapped LUA_IDSIZE can be placed alongside with the
introduced LUA_TRACEBAKLEVELS{1,2} for better constants locality.

> +#define LUA_IDSIZE_ERR		60  /* Size of error message */

I moved our corresponding discussion from the previous patch below.

|> > I don't get the reason to introduce a separate constant for a single
|> > line sources. Could you please provide a bit more extended rationale?
|>
|> The problem here is that if I allow 128 for size of error messages, all
|> error outputs will be longer and there will be need to fix vast amount
|> of test result outputs (.result files). Also, these long outputs will
|> flood tarantool output. I don't think there is a reason to increase
|> error message size. Do you?

I expect the following comment doesn't look offensive, but in case it does,
I definitely mean nothing abusive.

I still do not understand the reason to introduce a separate constant for
oneline sources.

|> there will be need to fix vast amount of test result outputs (.result
|> files).
AFAIR *.result' files can be adjusted automatically and committed with
the cmake related changes. Since the changes are trivial and atomic I
see nothing bad.

|> Also, these long outputs will flood tarantool output.
As well as long filenames, won't they?

|> Do you?
Yes, I do.

Foremost I see no reason to split the entity (i.e. short source name)
into a separate ones considering with respect to its origin (a source
file or a oneliner). IMHO, this change just introduces excess complexity
with no benefits.

Furthermore, there is no static assert and even comment regarding the
LUA_IDSIZE_ERR value: it have to be less or equal than LUA_IDSIZE. Since
the latter one can be tuned at compile-time there is no guarantee the
buffer overflow doesn't occur.

Considering all said above I'm strictly against introducing a separate
LUA_IDSIZE_ERR constant and urge you to revert the related changes.

>  /*
>  ** Size of lauxlib and io.* on-stack buffers. Weird workaround to avoid using
>  ** unreasonable amounts of stack space, but still retain ABI compatibility.
> diff --git a/test/tarantool-4581-traceback.test.lua b/test/tarantool-4581-traceback.test.lua
> new file mode 100755
> index 0000000..595871b
> --- /dev/null
> +++ b/test/tarantool-4581-traceback.test.lua
> @@ -0,0 +1,79 @@
> +#!/usr/bin/env tarantool
> +
> +---
> +--- gh-4581: Increase limits for debug.traceback
> +---
> +
> +local test = require('tap').test('traceback test')
> +test:plan(1)
> +
> +-- Currently there is no Lua way to detect the values of
> +-- LUA_TRACEBACK_LEVEL1, LUA_TRACEBACK_LEVEL2, LUA_IDSIZE. Therefore
> +-- taking into account the default values configured for Tarantool
> +-- build and considering the existing testing pipeline we can hardcode
> +-- the corresponding values within this test suite to check the output
> +-- produced via debug.backtrace.
> +local LUA_TRACEBACK_LEVEL1 = 25
> +local LUA_TRACEBACK_LEVEL2 = 25
> +
> +local TRACEBACK_MSG1 = 'recursion limit reached!'
> +local TRACEBACK_MSG2 = 'stack traceback:'
> +local TRACEBACK_PATTERN = 'tarantool%-4581%-traceback%.test%.lua:[0-9]+: in .*'
> +
> +-- We use local output variable instead of returning function
> +-- value recursively since luajit optimizes end recursion
> +-- and in that case we would only get 2 stack frames for traceback

Nice!

> +local output
> +
> +-- Recursive function which generates stack frames for debug.traceback
> +local function frec(rec_limit)
> +    if rec_limit <= 0 then
> +        output = debug.traceback(TRACEBACK_MSG1)
> +        -- Strip path from the output and return it
> +        output = output:gsub("[^\n]*/", "")
> +    else
> +        frec(rec_limit - 1)
> +    end
> +end
> +
> +-- Call debug.traceback with specified recursion depth
> +frec(100)
> +
> +local test_ok = false
> +
> +-- Split output into strings
> +local strings = {}
> +for each in output:gmatch("([^\n]+)") do
> +   table.insert(strings, each)
> +end
> +
> +-- Total output string count
> +local count = 0
> +    + 2 -- header
> +    + (LUA_TRACEBACK_LEVEL1-1) -- first part
> +    + 1 -- three dots
> +    + (LUA_TRACEBACK_LEVEL2) -- second part
> +
> +-- Check output strings
> +if count == table.getn(strings) then
> +    for i = 1,count,1 do

Minor: The third parameter is an excess one here, thus I guess it can be
dropped.

> +        local pattern
> +
> +        if i == 1 then 

Minor: Minor: A trailing whitespace is left above.

> +            pattern = TRACEBACK_MSG1
> +        elseif i == 2 then 

Minor: Minor: A trailing whitespace is left above.

> +            pattern = TRACEBACK_MSG2
> +        elseif i ~= LUA_TRACEBACK_LEVEL1 + 2 then
> +            pattern = TRACEBACK_PATTERN
> +        else
> +            pattern = '%.%.%.'
> +        end
> +
> +        test_ok = string.match(strings[i], pattern) ~= nil
> +        if not test_ok then
> +            break
> +        end
> +    end
> +end
> +
> +test:is(test_ok, true)
> -- 
> 2.17.1
> 

-- 
Best regards,
IM