From 3eabed74c0fca5dd9f96d2cca13c4a1a16d5c094 Mon Sep 17 00:00:00 2001 From: Matthew Ahrens Date: Mon, 27 Jul 2020 16:11:47 -0700 Subject: [PATCH] Fix lua stack overflow on recursive call to gsub() The `zfs program` subcommand invokes a LUA interpreter to run ZFS "channel programs". This interpreter runs in a constrained environment, with defined memory limits. The LUA stack (used for LUA functions that call each other) is allocated in the kernel's heap, and is limited by the `-m MEMORY-LIMIT` flag and the `zfs_lua_max_memlimit` module parameter. The C stack is used by certain LUA features that are implemented in C. The C stack is limited by `LUAI_MAXCCALLS=20`, which limits call depth. Some LUA C calls use more stack space than others, and `gsub()` uses an unusually large amount. With a programming trick, it can be invoked recursively using the C stack (rather than the LUA stack). This overflows the 16KB Linux kernel stack after about 11 iterations, less than the limit of 20. One solution would be to decrease `LUAI_MAXCCALLS`. This could be made to work, but it has a few drawbacks: 1. The existing test suite does not pass with `LUAI_MAXCCALLS=10`. 2. There may be other LUA functions that use a lot of stack space, and the stack space may change depending on compiler version and options. This commit addresses the problem by adding a new limit on the amount of free space (in bytes) remaining on the C stack while running the LUA interpreter: `LUAI_MINCSTACK=4096`. If there is less than this amount of stack space remaining, a LUA runtime error is generated. Reviewed-by: George Wilson Reviewed-by: Brian Behlendorf Reviewed-by: Ryan Moeller Reviewed-by: Allan Jude Reviewed-by: Serapheim Dimitropoulos Signed-off-by: Matthew Ahrens Closes #10611 Closes #10613 --- module/lua/ldebug.c | 2 ++ module/lua/ldo.c | 25 +++++++++++++- module/lua/llimits.h | 6 ++++ module/lua/lstate.c | 1 + module/lua/lstate.h | 1 + tests/runfiles/common.run | 2 +- tests/zfs-tests/include/tunables.cfg | 1 + .../channel_program/lua_core/Makefile.am | 3 ++ .../lua_core/tst.memory_limit.ksh | 3 ++ .../lua_core/tst.stack_gsub.err | 18 ++++++++++ .../lua_core/tst.stack_gsub.ksh | 33 +++++++++++++++++++ .../lua_core/tst.stack_gsub.zcp | 20 +++++++++++ 12 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.err create mode 100755 tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.ksh create mode 100644 tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.zcp diff --git a/module/lua/ldebug.c b/module/lua/ldebug.c index 32bb908cd5..2e1efa4e72 100644 --- a/module/lua/ldebug.c +++ b/module/lua/ldebug.c @@ -598,10 +598,12 @@ l_noret luaG_errormsg (lua_State *L) { l_noret luaG_runerror (lua_State *L, const char *fmt, ...) { + L->runerror++; va_list argp; va_start(argp, fmt); addinfo(L, luaO_pushvfstring(L, fmt, argp)); va_end(argp); luaG_errormsg(L); + L->runerror--; } /* END CSTYLED */ diff --git a/module/lua/ldo.c b/module/lua/ldo.c index 88b22836f9..0344a29fd8 100644 --- a/module/lua/ldo.c +++ b/module/lua/ldo.c @@ -29,6 +29,24 @@ +/* Return the number of bytes available on the stack. */ +#if defined (_KERNEL) && defined(__linux__) +#include +static intptr_t stack_remaining(void) { + char local; + return (intptr_t)(&local - (char *)current->stack); +} +#elif defined (_KERNEL) && defined(__FreeBSD__) +#include +static intptr_t stack_remaining(void) { + char local; + return (intptr_t)(&local - (char *)curthread->td_kstack); +} +#else +static intptr_t stack_remaining(void) { + return INTPTR_MAX; +} +#endif /* ** {====================================================== @@ -445,8 +463,13 @@ void luaD_call (lua_State *L, StkId func, int nResults, int allowyield) { if (L->nCcalls == LUAI_MAXCCALLS) luaG_runerror(L, "C stack overflow"); else if (L->nCcalls >= (LUAI_MAXCCALLS + (LUAI_MAXCCALLS>>3))) - luaD_throw(L, LUA_ERRERR); /* error while handing stack error */ + luaD_throw(L, LUA_ERRERR); /* error while handling stack error */ } + intptr_t remaining = stack_remaining(); + if (L->runerror == 0 && remaining < LUAI_MINCSTACK) + luaG_runerror(L, "C stack overflow"); + if (L->runerror != 0 && remaining < LUAI_MINCSTACK / 2) + luaD_throw(L, LUA_ERRERR); /* error while handling stack error */ if (!allowyield) L->nny++; if (!luaD_precall(L, func, nResults)) /* is a Lua function? */ luaV_execute(L); /* call it */ diff --git a/module/lua/llimits.h b/module/lua/llimits.h index b989160f41..177092fbc2 100644 --- a/module/lua/llimits.h +++ b/module/lua/llimits.h @@ -122,6 +122,12 @@ typedef LUAI_UACNUMBER l_uacNumber; #define LUAI_MAXCCALLS 20 #endif +/* + * Minimum amount of available stack space (in bytes) to make a C call. With + * gsub() recursion, the stack space between each luaD_call() is 1256 bytes. + */ +#define LUAI_MINCSTACK 4096 + /* ** maximum number of upvalues in a closure (both C and Lua). (Value ** must fit in an unsigned char.) diff --git a/module/lua/lstate.c b/module/lua/lstate.c index 1b1d948fac..4d196eced6 100644 --- a/module/lua/lstate.c +++ b/module/lua/lstate.c @@ -214,6 +214,7 @@ static void preinit_state (lua_State *L, global_State *g) { L->nny = 1; L->status = LUA_OK; L->errfunc = 0; + L->runerror = 0; } diff --git a/module/lua/lstate.h b/module/lua/lstate.h index 22e575e9a2..b636396a60 100644 --- a/module/lua/lstate.h +++ b/module/lua/lstate.h @@ -166,6 +166,7 @@ struct lua_State { unsigned short nCcalls; /* number of nested C calls */ lu_byte hookmask; lu_byte allowhook; + lu_byte runerror; /* handling a runtime error */ int basehookcount; int hookcount; lua_Hook hook; diff --git a/tests/runfiles/common.run b/tests/runfiles/common.run index 61c0952906..931eee4faf 100644 --- a/tests/runfiles/common.run +++ b/tests/runfiles/common.run @@ -74,7 +74,7 @@ tests = ['tst.args_to_lua', 'tst.divide_by_zero', 'tst.exists', 'tst.memory_limit', 'tst.nested_neg', 'tst.nested_pos', 'tst.nvlist_to_lua', 'tst.recursive_neg', 'tst.recursive_pos', 'tst.return_large', 'tst.return_nvlist_neg', 'tst.return_nvlist_pos', - 'tst.return_recursive_table', 'tst.timeout'] + 'tst.return_recursive_table', 'tst.stack_gsub', 'tst.timeout'] tags = ['functional', 'channel_program', 'lua_core'] [tests/functional/channel_program/synctask_core] diff --git a/tests/zfs-tests/include/tunables.cfg b/tests/zfs-tests/include/tunables.cfg index df8824b1ec..9191f29559 100644 --- a/tests/zfs-tests/include/tunables.cfg +++ b/tests/zfs-tests/include/tunables.cfg @@ -35,6 +35,7 @@ DISABLE_IVSET_GUID_CHECK disable_ivset_guid_check zfs_disable_ivset_guid_check INITIALIZE_CHUNK_SIZE initialize_chunk_size zfs_initialize_chunk_size INITIALIZE_VALUE initialize_value zfs_initialize_value KEEP_LOG_SPACEMAPS_AT_EXPORT keep_log_spacemaps_at_export zfs_keep_log_spacemaps_at_export +LUA_MAX_MEMLIMIT lua.max_memlimit zfs_lua_max_memlimit L2ARC_NOPREFETCH l2arc.noprefetch l2arc_noprefetch L2ARC_REBUILD_BLOCKS_MIN_L2SIZE l2arc.rebuild_blocks_min_l2size l2arc_rebuild_blocks_min_l2size L2ARC_REBUILD_ENABLED l2arc.rebuild_enabled l2arc_rebuild_enabled diff --git a/tests/zfs-tests/tests/functional/channel_program/lua_core/Makefile.am b/tests/zfs-tests/tests/functional/channel_program/lua_core/Makefile.am index e06b145dcc..fb35208119 100644 --- a/tests/zfs-tests/tests/functional/channel_program/lua_core/Makefile.am +++ b/tests/zfs-tests/tests/functional/channel_program/lua_core/Makefile.am @@ -21,6 +21,7 @@ dist_pkgdata_SCRIPTS = \ tst.return_nvlist_neg.ksh \ tst.return_nvlist_pos.ksh \ tst.return_recursive_table.ksh \ + tst.stack_gsub.ksh \ tst.timeout.ksh dist_pkgdata_DATA = \ @@ -40,4 +41,6 @@ dist_pkgdata_DATA = \ tst.recursive.zcp \ tst.return_large.zcp \ tst.return_recursive_table.zcp \ + tst.stack_gsub.err \ + tst.stack_gsub.zcp \ tst.timeout.zcp diff --git a/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.memory_limit.ksh b/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.memory_limit.ksh index 2885775686..0533b8fa30 100755 --- a/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.memory_limit.ksh +++ b/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.memory_limit.ksh @@ -61,6 +61,9 @@ log_mustnot_checkerror_program "Memory limit exhausted" -m 1 $TESTPOOL - <<-EOF return s EOF +# Set the memlimit, in case it is a non-default value +log_must set_tunable32 LUA_MAX_MEMLIMIT 100000000 + log_mustnot_checkerror_program "Invalid instruction or memory limit" \ -m 200000000 $TESTPOOL - <<-EOF return 1; diff --git a/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.err b/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.err new file mode 100644 index 0000000000..45f2d9ef0e --- /dev/null +++ b/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.err @@ -0,0 +1,18 @@ +Channel program execution failed: +C stack overflow +stack traceback: + [C]: in function 'gsub' + [string "channel program"]:17: in function <[string "channel program"]:16> + [C]: in function 'gsub' + [string "channel program"]:17: in function <[string "channel program"]:16> + [C]: in function 'gsub' + [string "channel program"]:17: in function <[string "channel program"]:16> + [C]: in function 'gsub' + [string "channel program"]:17: in function <[string "channel program"]:16> + [C]: in function 'gsub' + [string "channel program"]:17: in function <[string "channel program"]:16> + [C]: in function 'gsub' + [string "channel program"]:17: in function <[string "channel program"]:16> + [C]: in function 'gsub' + [string "channel program"]:17: in function <[string "channel program"]:16> + (...tail calls...) \ No newline at end of file diff --git a/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.ksh b/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.ksh new file mode 100755 index 0000000000..ecabf3a3fe --- /dev/null +++ b/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.ksh @@ -0,0 +1,33 @@ +#!/bin/ksh -p +# +# This file and its contents are supplied under the terms of the +# Common Development and Distribution License ("CDDL"), version 1.0. +# You may only use this file in accordance with the terms of version +# 1.0 of the CDDL. +# +# A full copy of the text of the CDDL should have accompanied this +# source. A copy of the CDDL is also available via the Internet at +# http://www.illumos.org/license/CDDL. +# + +# +# Copyright (c) 2020 by Delphix. All rights reserved. +# + +. $STF_SUITE/tests/functional/channel_program/channel_common.kshlib + +# +# DESCRIPTION: +# Overflowing the C stack using recursive gsub() should be handled +# gracefully. gsub() uses more stack space than typical, so it relies +# on LUAI_MINCSTACK to ensure that we don't overflow the Linux kernel's +# stack. +# + +verify_runnable "global" + +log_assert "recursive gsub() should be handled gracefully" + +log_mustnot_program $TESTPOOL $ZCP_ROOT/lua_core/tst.stack_gsub.zcp + +log_pass "recursive gsub() should be handled gracefully" diff --git a/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.zcp b/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.zcp new file mode 100644 index 0000000000..a493363ca6 --- /dev/null +++ b/tests/zfs-tests/tests/functional/channel_program/lua_core/tst.stack_gsub.zcp @@ -0,0 +1,20 @@ +-- +-- This file and its contents are supplied under the terms of the +-- Common Development and Distribution License ("CDDL"), version 1.0. +-- You may only use this file in accordance with the terms of version +-- 1.0 of the CDDL. +-- +-- A full copy of the text of the CDDL should have accompanied this +-- source. A copy of the CDDL is also available via the Internet at +-- http://www.illumos.org/license/CDDL. +-- + +-- +-- Copyright (c) 2020 by Delphix. All rights reserved. +-- + +function f(s) + return string.gsub(s, ".", f) +end + +return f("foo")