Fix string leak in aegisub.ffi

[filip-hejsek]

This function leaks the string passed to it:

-- Convert a const char * to a lua string, returning nil rather than crashing
-- if it's NULL and freeing the input if it's non-const
string = (cdata) ->
  return nil if cdata == nil
  str = ffi.string cdata
  if type(cdata) == char_ptr
    ffi.C.free cdata
  str

This is because the const check is broken as type(cdata) just returns 'cdata' (the correct function is ffi.typeof).

---

Are we sure we want this function (and the whole aegisub.ffi module) to be exposed to automation scripts? I don't think it's supposed to be public API, but it's still possible to use it from scripts. It doesn't seem anyone actually uses it, but if someone in fact does and relies on the broken behavior, they're going to have a lot of fun debugging double frees.

---

Leak found by LSan:

==301026==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 5677 byte(s) in 223 object(s) allocated from:
    #0 0x5572b4ba2ac5 in malloc (/home/filiph/git/Aegisub/build_sanitizer/aegisub+0x748ac5) (BuildId: f57b0bb04eb23c2269ae505d9789e73446c90808)
    #1 0x5572b4e5b2f0 in char* agi::lua::strndup<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) /home/filiph/git/Aegisub/build_sanitizer/../libaegisub/include/libaegisub/lua/ffi.h:51:33
    #2 0x5572b5800f7a in (anonymous namespace)::dir_next(agi::fs::DirectoryIterator&, char**)::$_0::operator()() const /home/filiph/git/Aegisub/build_sanitizer/../libaegisub/lua/modules/lfs.cpp:81:14
    #3 0x5572b5800f7a in decltype(fp0()) (anonymous namespace)::wrap<(anonymous namespace)::dir_next(agi::fs::DirectoryIterator&, char**)::$_0>(char**, (anonymous namespace)::dir_next(agi::fs::DirectoryIterator&, char**)::$_0) /home/filiph/git/Aegisub/build_sanitizer/../libaegisub/lua/modules/lfs.cpp:34:10
    #4 0x5572b5800f7a in (anonymous namespace)::dir_next(agi::fs::DirectoryIterator&, char**) /home/filiph/git/Aegisub/build_sanitizer/../libaegisub/lua/modules/lfs.cpp:80:9
    #5 0x5572b5676668 in lj_vm_ffi_call /home/filiph/git/Aegisub/build_sanitizer/subprojects/LuaJIT-04dca7911ea255f37be799c18d74c305b921c1a6/src/lj_vm.S:2709
    #6 0x5572b566c1a6 in lj_ccall_func /home/filiph/git/Aegisub/build_sanitizer/../subprojects/LuaJIT-04dca7911ea255f37be799c18d74c305b921c1a6/src/lj_ccall.c:1187:5
    #7 0x5572b566c1a6 in lj_cf_ffi_meta___call /home/filiph/git/Aegisub/build_sanitizer/../subprojects/LuaJIT-04dca7911ea255f37be799c18d74c305b921c1a6/src/lib_ffi.c:230:15

Direct leak of 1158 byte(s) in 45 object(s) allocated from:
    #0 0x5572b4ba2ac5 in malloc (/home/filiph/git/Aegisub/build_sanitizer/aegisub+0x748ac5) (BuildId: f57b0bb04eb23c2269ae505d9789e73446c90808)
    #1 0x5572b4e5b2f0 in char* agi::lua::strndup<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>>>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) /home/filiph/git/Aegisub/build_sanitizer/../libaegisub/include/libaegisub/lua/ffi.h:51:33
    #2 0x5572b5800f7a in (anonymous namespace)::dir_next(agi::fs::DirectoryIterator&, char**)::$_0::operator()() const /home/filiph/git/Aegisub/build_sanitizer/../libaegisub/lua/modules/lfs.cpp:81:14
    #3 0x5572b5800f7a in decltype(fp0()) (anonymous namespace)::wrap<(anonymous namespace)::dir_next(agi::fs::DirectoryIterator&, char**)::$_0>(char**, (anonymous namespace)::dir_next(agi::fs::DirectoryIterator&, char**)::$_0) /home/filiph/git/Aegisub/build_sanitizer/../libaegisub/lua/modules/lfs.cpp:34:10
    #4 0x5572b5800f7a in (anonymous namespace)::dir_next(agi::fs::DirectoryIterator&, char**) /home/filiph/git/Aegisub/build_sanitizer/../libaegisub/lua/modules/lfs.cpp:80:9
    #5 0x5572b956328e  (<unknown module>)

[arch1t3cht]

Thanks!

And no, I don't think aegisub.ffi should be considered public API. I don't know of any script that relies on it either.

(To be honest, I also don't know why most of Aegisub's lua modules use ffi in the first place. The modules were refactored to using ffi in 2014 but I can't see any reasoning for it in the commit. It may be a bit faster and/or get rid of some boilerplate code, but it has the big disadvantage of locking Aegisub into a strict luajit dependency that cannot be replaced with normal lua. But figuring this out and potentially improving this has been quite low priority for me so far given all the other issues.)

[filip-hejsek]

And no, I don't think aegisub.ffi should be considered public API. I don't know of any script that relies on it either.

We might consider moving it to aegisub.internal.ffi to make that clear. #560