Lua FFI fixes (#657)

* Fix clipboard.get leaking its buffer on every call

clipboard_get returns strndup'd memory but typed it const char *, and
aegisub.internal.ffi's string helper deliberately only frees non-const
char * (const means the native side retains ownership). Type it char *
so the buffer is actually freed.

* Fix leaking the error message when re.compile fails

regex_compile strdups the exception text into the error out-param, but
the moon side read it with ffi.string, which does not free. Use
ffi_util.string, which does.

* Don't let C++ exceptions escape through the FFI boundary in re

boost::regex can throw -- most notably on invalid UTF-8 via the
UTF-8-to-UTF-32 iterators (the crash backtrace in Aegisub#99),
and on regex complexity/backtracking limits -- but only regex_compile
caught anything. An exception unwinding through the LuaJIT interpreter
or JIT frames which called these functions is undefined behavior and
can take down the process.

Catch exceptions in regex_search/regex_match/regex_replace and report
them through a char **err out-param as lfs already does; the moon side
raises them as proper Lua errors.

* Return an error from unicode case conversion on invalid UTF-8

On a bogus conversion the wrapper set the error out-param but then ran
the conversion anyway and returned a result, which the moon side's
error branch never freed. Return null once the error is set; the moon
side already handles a null result plus an error message correctly.

* Check for malloc failure in strndup

Previously OOM meant memcpy through a null pointer. The Lua side
already maps a null return to nil.

* Fix lfs.chdir/mkdir/rmdir/touch always returning nil

The impl functions return a C bool, which LuaJIT's ffi converts to a
Lua boolean, and tonumber(true) is nil -- so these four functions
returned nil, nil on success, indistinguishable from failure to any
caller checking the standard lfs true-or-nil,err convention. Found
because busted's own chdir call refuses to run the automation test
suite against the aegisub lfs.

* Fix swapped error() arguments in lfs.dir

error 2, err raises the number 2 with err as the level; failing to open
a directory should raise the error message.

* Remove the uuid dependency from the lfs tests

It was only used to generate unique names under /tmp, which os.tmpname
does without an external rock (whose API has also drifted: uuid 1.0
refuses to run until an rng is explicitly configured). os.tmpname
creates the file on POSIX, so unlink it and use just the name.

* meson: build aegisub-lua and wire up the automation tests

Builds the standalone Lua host from automation/tests and registers the
busted suite as a meson test when busted is installed through luarocks
for Lua 5.1; nothing has been running these tests since Travis went
away. The suite's fixtures are POSIX-only, so it is only registered on
non-Windows hosts. Run with: meson test --suite automation

* Document that Windows builds require the dynamic CRT

This has been institutional knowledge since the /MT era of the old
Visual Studio build broke automation (2016-2018); write it down where
the next person flipping b_vscrt will find it.

* ci: install busted and run the full test suite

The test step only ran "gtest main" by name, so luajit-52 (and now the
automation suite) never ran in CI. Install busted through luarocks on
Ubuntu and macOS so the automation tests are registered; Windows keeps
skipping them since luarocks has no painless setup there and the meson
gate handles the absence.

* Add test coverage for lfs.dir and invalid UTF-8 handling

lfs.dir had no tests at all; the re specs pin that invalid UTF-8 is
reported as a Lua error, which is the path that previously crashed
(Aegisub#99); the unicode spec pins ICU's substitution
behavior for invalid input. Notably lfs.dir's error branch turns out
to be unreachable: DirectoryIterator swallows construction failures
on every platform and yields an empty iteration instead.

* Fix re.compile raising the module table instead of the error message

real_compile passed the FFI impl module to error() rather than the
message returned from compile, so failed compiles raised a useless
table. The existing invalid-regex test only checked that an error was
raised, so extend it to check the error is actually a string.

* Consolidate the re exception guards into a catch-all wrap helper

Mirror the wrap() idiom lfs.cpp already uses: a single helper with a
catch-all arm, so no exception type can unwind through LuaJIT frames
even if boost throws something not derived from std::exception. Also
check the malloc in regex_search like strndup already does.

* Remove test asserting lfs.dir's silent behavior on unopenable paths

Vanilla lfs errors out from lfs.dir when the directory can't be
opened, and require 'lfs' is transparently redirected to aegisub.lfs,
so the silent empty iteration is a compatibility bug rather than
behavior to enshrine in a test. To be fixed separately.

* ci: pin meson test to our own suites and require the automation suite

Bare meson test also runs the test suites of subproject dependencies
(the uchardet wrap alone adds 111 tests on macOS), so limit it to the
Aegisub project's tests. Since the automation suite is only registered
when the busted probe succeeds at configure time, fail the build if it
silently drops out rather than reporting green without it.

Author: CoffeeFlux

.github/workflows/ci.yml

@@ -132,12 +132,20 @@ jobs:
           brew install libass zlib ffms2 fftw hunspell
           brew install pulseaudio  # NO OpenAL in github CI
 
+          # busted for the automation test suite (LuaJIT is Lua 5.1 to luarocks)
+          brew install luajit luarocks
+          luarocks --lua-dir="$(brew --prefix luajit)" --lua-version=5.1 install busted
+
       - name: Install dependencies (Linux)
         if: startsWith(matrix.config.os, 'ubuntu-')
         run: |
           sudo apt-get update
           sudo apt-get install ninja-build build-essential libx11-dev libfreetype6-dev pkg-config libfontconfig1-dev libass-dev libasound2-dev libffms2-dev intltool libboost-all-dev libhunspell-dev libcurl4-openssl-dev libuchardet-dev libgtest-dev libgmock-dev libwxgtk3.2-dev libportal-gtk3-dev
 
+          # busted for the automation test suite (LuaJIT is Lua 5.1 to luarocks)
+          sudo apt-get install -y lua5.1 liblua5.1-0-dev luarocks
+          sudo luarocks --lua-version=5.1 install busted
+
       - name: Configure
         run: |
           ${{ matrix.config.devenv }}
@@ -148,10 +156,17 @@ jobs:
           ${{ matrix.config.devenv }}
           meson compile -C build
 
+      # The automation suite is registered at configure time only if the
+      # busted probe succeeds, so make sure it can't silently drop out of CI
+      - name: Check automation tests are registered
+        if: ${{ !startsWith(matrix.config.os, 'windows-') }}
+        run: meson test -C build --list | grep automation
+
+      # --suite limits this to our own tests, excluding those of subproject deps
       - name: Run test
         run: |
           ${{ matrix.config.devenv }}
-          meson test -C build --verbose "gtest main"
+          meson test -C build --verbose --suite Aegisub
 
       # Windows artifacts
       - name: Generate Windows installer

automation/include/aegisub/lfs.moon

@@ -27,9 +27,10 @@ string_ret = (f) -> (...) ->
   res, err = f ...
   ffi_util.string(res), err
 
-number_ret = (f) -> (...) ->
+-- Convert the C bool returns to lfs's true-or-nil,err convention
+bool_ret = (f) -> (...) ->
   res, err = f ...
-  tonumber(res), err
+  if res then true else nil, err
 
 attributes = check'string ?string' (path, field) ->
   switch field
@@ -67,16 +68,16 @@ class dir_iter
 dir = check'string' (path) ->
   obj, err = impl.dir_new path
   if err
-    error 2, err
+    error err, 2
   iter = dir_iter obj
   iter.next, iter
 
 return {
   :attributes
-  chdir: check'string' number_ret impl.chdir
+  chdir: check'string' bool_ret impl.chdir
   currentdir: check'' string_ret impl.currentdir
   :dir
-  mkdir: check'string' number_ret impl.mkdir
-  rmdir: check'string'number_ret impl.rmdir
-  touch: check'string'number_ret impl.touch
+  mkdir: check'string' bool_ret impl.mkdir
+  rmdir: check'string'bool_ret impl.rmdir
+  touch: check'string'bool_ret impl.touch
 }

automation/include/aegisub/re.moon

@@ -33,21 +33,35 @@ regex_flag = ffi.typeof 'agi_re_flag'
 -- Get the boost::eegex binding
 regex = require 'aegisub.__re_impl'
 
+err_buff = ffi.new 'char *[1]'
+
+-- Raise a Lua error if the last native call reported one
+check_err = ->
+  if err_buff[0] != nil
+    error ffi_util.string(err_buff[0]), 3
+
 -- Wrappers to convert returned values from C types to Lua types
 search = (re, str, start) ->
   return unless start <= str\len()
-  res = regex.search re, str, str\len(), start
+  err_buff[0] = nil
+  res = regex.search re, str, str\len(), start, err_buff
+  check_err!
   return unless res != nil
   first, last = res[0], res[1]
   ffi.gc(res, ffi.C.free)
   first, last
 
 replace = (re, replacement, str, max_count) ->
-  ffi_util.string regex.replace re, replacement, str, str\len(), max_count
+  err_buff[0] = nil
+  res = regex.replace re, replacement, str, str\len(), max_count, err_buff
+  check_err!
+  ffi_util.string res
 
 match = (re, str, start) ->
   assert start <= str\len()
-  m = regex.match re, str, str\len(), start
+  err_buff[0] = nil
+  m = regex.match re, str, str\len(), start, err_buff
+  check_err!
   return unless m != nil
   ffi.gc m, regex.match_free
 
@@ -56,12 +70,11 @@ get_match = (m, idx) ->
   return unless res != nil
   res[0], res[1] -- Result buffer is owned by match so no need to free
 
-err_buff = ffi.new 'char *[1]'
 compile = (pattern, flags) ->
   err_buff[0] = nil
   re = regex.compile pattern, flags, err_buff
   if err_buff[0] != nil
-    return ffi.string err_buff[0]
+    return ffi_util.string err_buff[0]
   ffi.gc re, regex.regex_free
 
 -- Return the first n elements from ...
@@ -234,7 +247,7 @@ real_compile = (pattern, level, flags, stored_level) ->
 
   re = compile pattern, flags
   if type(re) == 'string'
-    error regex, level + 1
+    error re, level + 1
 
   RegEx re, stored_level or level + 1
 

automation/tests/meson.build

@@ -0,0 +1,37 @@
+aegisub_lua_args = []
+# force UTF8 for MSVC
+if host_machine.system() == 'windows'
+    aegisub_lua_args += ['/utf-8']
+endif
+
+aegisub_lua = executable(
+    'aegisub-lua',
+    'aegisub.cpp',
+    include_directories: libaegisub_inc,
+    dependencies: deps,
+    cpp_args: aegisub_lua_args + conf_defines,
+    link_with: libaegisub,
+)
+
+# Requires busted installed through luarocks for Lua 5.1; the fixtures are
+# POSIX-only, so the suite is never registered on Windows
+have_busted = false
+if host_machine.system() != 'windows'
+    luarocks = find_program('luarocks', required: false)
+    if luarocks.found()
+        have_busted = run_command(luarocks, '--lua-version=5.1', 'show', 'busted', check: false).returncode() == 0
+    endif
+endif
+
+if have_busted
+    lr_path = run_command(luarocks, '--lua-version=5.1', 'path', '--lr-path', check: false).stdout().strip()
+    lr_cpath = run_command(luarocks, '--lua-version=5.1', 'path', '--lr-cpath', check: false).stdout().strip()
+    test('automation',
+        aegisub_lua,
+        args: ['tests/busted.lua', '-p', 'moon', 'tests/modules'],
+        workdir: meson.current_source_dir() / '..',
+        env: {'LUA_PATH': lr_path + ';;', 'LUA_CPATH': lr_cpath + ';;'},
+        suite: 'automation')
+else
+    message('luarocks with busted for Lua 5.1 not found; not adding the automation test suite')
+endif

automation/tests/modules/lfs.moon

@@ -13,9 +13,12 @@
 -- OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 lfs = require 'aegisub.lfs'
-uuid = require 'uuid'
 
-uuid.randomseed os.time()
+-- os.tmpname creates the file on POSIX; we only want a unique name
+temp_name = ->
+  name = os.tmpname!
+  os.remove name
+  name
 
 get_pwd = ->
   pwd = io.popen 'pwd'
@@ -44,15 +47,15 @@ describe 'lfs', ->
       assert.is.equal get_pwd!, dir
 
     it 'should fail on an invalid path', ->
-      name = '/tmp/' .. uuid! .. '/' .. uuid!
+      name = temp_name! .. '/child'
       res, msg = lfs.chdir name
 
       assert.is.nil res
       assert.is.not.nil msg
 
   describe 'mkdir', ->
     it 'should be able to create new directories', ->
-      name = '/tmp/' .. uuid!
+      name = temp_name!
       lfs.mkdir name
       assert.is.equal lfs.attributes(name, 'mode'), 'directory'
 
@@ -61,9 +64,24 @@ describe 'lfs', ->
 
   describe 'touch', ->
     it 'should create files if given a nonexistent filename', ->
-      name = '/tmp/' .. uuid!
+      name = temp_name!
       lfs.touch name
       assert.is.equal lfs.attributes(name).mode, 'file'
 
       os.remove(name)
       assert.is.nil lfs.attributes name, 'mode'
+
+  describe 'dir', ->
+    it 'should iterate over the files in a directory', ->
+      name = temp_name!
+      lfs.mkdir name
+      lfs.touch name .. '/a'
+      lfs.touch name .. '/b'
+
+      files = [f for f in lfs.dir name]
+      table.sort files
+      assert.is.same {'a', 'b'}, files
+
+      os.remove name .. '/a'
+      os.remove name .. '/b'
+      lfs.rmdir name

automation/tests/modules/re.moon

@@ -33,6 +33,11 @@ describe 'compile', ->
   it 'should throw an error when given an invalid regex', ->
     assert.is.error -> re.compile '('
 
+  it 'should throw the compile error message for an invalid regex', ->
+    ok, err = pcall -> re.compile '('
+    assert.is.false ok
+    assert.is.equal 'string', type err
+
   it 'should throw an error when given an empty regex', ->
     assert.is.error -> re.compile ''
 
@@ -326,3 +331,14 @@ describe 'sub', ->
     assert.is.not.nil res
     assert.is.equal 'dadbdcd', res
 
+describe 'invalid UTF-8', ->
+  it 'should be reported as an error by find', ->
+    ok, err = pcall -> re.find 'abc\255def', 'd'
+    assert.is.false ok
+    assert.is.truthy err\find 'UTF-8', 1, true
+
+  it 'should be reported as an error by match', ->
+    ok, err = pcall -> re.match '\255', 'x'
+    assert.is.false ok
+    assert.is.truthy err\find 'UTF-8', 1, true
+

automation/tests/modules/unicode.moon

@@ -59,6 +59,8 @@ describe 'to_upper_case', ->
     assert.is.equal 'SS', unicode.to_upper_case 'ß'
   it 'should support ligatures', ->
     assert.is.equal 'FFI', unicode.to_upper_case 'ffi'
+  it 'should replace invalid UTF-8 with the replacement character', ->
+    assert.is.equal '�', unicode.to_upper_case '\255'
 
 describe 'to_lower_case', ->
   it 'should support plain ASCII', ->

libaegisub/include/libaegisub/lua/ffi.h

@@ -49,6 +49,7 @@ void register_lib_table(lua_State *L, std::initializer_list<const char *> types,
 template<typename T>
 char *strndup(T const& str) {
 	char *ret = static_cast<char*>(malloc(str.size() + 1));
+	if (!ret) return nullptr;
 	memcpy(ret, str.data(), str.size());
 	ret[str.size()] = 0;
 	return ret;

libaegisub/lua/modules/re.cpp

@@ -47,11 +47,30 @@ bool search(u32regex& re, const char *str, size_t len, int start, boost::cmatch&
 		start > 0 ? boost::match_prev_avail | boost::match_not_bob : boost::match_default);
 }
 
-match *regex_match(u32regex& re, const char *str, size_t len, int start) {
-	auto result = std::make_unique<match>();
-	if (!search(re, str, len, start, result->m))
-		return nullptr;
-	return result.release();
+// boost::regex can throw (e.g. on invalid UTF-8), and exceptions must not
+// unwind through LuaJIT frames, so trap everything and report errors through
+// an out-param
+template<typename Func>
+auto wrap(char **err, Func f) -> decltype(f()) {
+	try {
+		return f();
+	}
+	catch (std::exception const& e) {
+		*err = strdup(e.what());
+	}
+	catch (...) {
+		*err = strdup("Unknown error");
+	}
+	return decltype(f()){};
+}
+
+match *regex_match(u32regex& re, const char *str, size_t len, int start, char **err) {
+	return wrap(err, [&]() -> match * {
+		auto result = std::make_unique<match>();
+		if (!search(re, str, len, start, result->m))
+			return nullptr;
+		return result.release();
+	});
 }
 
 int *regex_get_match(match& match, size_t idx) {
@@ -62,48 +81,49 @@ int *regex_get_match(match& match, size_t idx) {
 	return match.range;
 }
 
-int *regex_search(u32regex& re, const char *str, size_t len, size_t start) {
-	boost::cmatch result;
-	if (!search(re, str, len, start, result))
-		return nullptr;
-
-	auto ret = static_cast<int *>(malloc(sizeof(int) * 2));
-	ret[0] = start + result.position() + 1;
-	ret[1] = start + result.position() + result.length();
-	return ret;
+int *regex_search(u32regex& re, const char *str, size_t len, size_t start, char **err) {
+	return wrap(err, [&]() -> int * {
+		boost::cmatch result;
+		if (!search(re, str, len, start, result))
+			return nullptr;
+
+		auto ret = static_cast<int *>(malloc(sizeof(int) * 2));
+		if (!ret) return nullptr;
+		ret[0] = start + result.position() + 1;
+		ret[1] = start + result.position() + result.length();
+		return ret;
+	});
 }
 
-char *regex_replace(u32regex& re, const char *replacement, const char *str, size_t len, int max_count) {
-	// Can't just use regex_replace here since it can only do one or infinite replacements
-	auto match = boost::u32regex_iterator<const char *>(str, str + len, re);
-	auto end_it = boost::u32regex_iterator<const char *>();
-
-	auto suffix = str;
-
-	std::string ret;
-	auto out = back_inserter(ret);
-	while (match != end_it && max_count > 0) {
-		copy(suffix, match->prefix().second, out);
-		match->format(out, replacement);
-		suffix = match->suffix().first;
-		++match;
-		--max_count;
-	}
-
-	ret += suffix;
-	return agi::lua::strndup(ret);
+char *regex_replace(u32regex& re, const char *replacement, const char *str, size_t len, int max_count, char **err) {
+	return wrap(err, [&]() -> char * {
+		// Can't just use regex_replace here since it can only do one or infinite replacements
+		auto match = boost::u32regex_iterator<const char *>(str, str + len, re);
+		auto end_it = boost::u32regex_iterator<const char *>();
+
+		auto suffix = str;
+
+		std::string ret;
+		auto out = back_inserter(ret);
+		while (match != end_it && max_count > 0) {
+			copy(suffix, match->prefix().second, out);
+			match->format(out, replacement);
+			suffix = match->suffix().first;
+			++match;
+			--max_count;
+		}
+
+		ret += suffix;
+		return agi::lua::strndup(ret);
+	});
 }
 
 u32regex *regex_compile(const char *pattern, int flags, char **err) {
-	auto re = std::make_unique<u32regex>();
-	try {
+	return wrap(err, [&]() -> u32regex * {
+		auto re = std::make_unique<u32regex>();
 		*re = boost::make_u32regex(pattern, boost::u32regex::perl | flags);
 		return re.release();
-	}
-	catch (std::exception const& e) {
-		*err = strdup(e.what());
-		return nullptr;
-	}
+	});
 }
 
 void regex_free(u32regex *re) { delete re; }