Returning nullptr from LV2 assertions may cause crashes in plugins

[swesterfeld]

I've seen in the two plugins that I wrote - liquidsfz and SpectMorph - that you could easily get crashes like this:

[...]
[carla] Carla assertion failure: "absolute_path != nullptr && absolute_path[0] != '\0'" in file CarlaPluginLV2.cpp, line 7736

Thread 1 "zrythm" received signal SIGSEGV, Segmentation fault.
__strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:74
74      ../sysdeps/x86_64/multiarch/strlen-avx2.S: Datei oder Verzeichnis nicht gefunden.
Warning: 'set logging on', an alias for the command 'set logging enabled', is deprecated.
Use 'set logging enabled on'.

SIGTRAP is used by the debugger.
Are you sure you want to change it? (y or n) [answered Y; input not from terminal]
backtrace:
#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:74
No locals.
#1  0x00007fffc74ded79 in std::char_traits<char>::length (__s=0x0) at /usr/include/c++/11/bits/char_traits.h:399
No locals.
#2  std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::assign (__s=0x0, this=0x7fffffffb3a0) at /usr/include/c++/11/bits/basic_string.h:1459
No locals.
#3  std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::operator= (__s=0x0, this=0x7fffffffb3a0) at /usr/include/c++/11/bits/basic_string.h:690
No locals.
#4  (anonymous namespace)::LV2Plugin::save (features=<optimized out>, handle=0x55558fe2f460, store=0x7ffff618a580 <CarlaBackend::CarlaPluginLV2::carla_lv2_state_store(void*, unsigned int, void const*, unsigned long, unsigned int, unsigned int)>, this=0x55558628a600) at ./lv2plugin.cc:405
        abstract_path = 0x0 
        map_path = <optimized out>
        path = {_M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>}, _M_p = 0x7fffffffb3b0 ""}, _M_string_length = 0, {_M_local_buf = '\000' <wiederholt 15 Mal>, _M_allocated_capacity = 0}}
        map_path = <optimized out>
        path = <optimized out>
        i = <optimized out>
        abstract_path = <optimized out>
#5  save (instance=0x55558628a600, store=0x7ffff618a580 <CarlaBackend::CarlaPluginLV2::carla_lv2_state_store(void*, unsigned int, void const*, unsigned long, unsigned int, unsigned int)>, handle=0x55558fe2f460, flags=<optimized out>, features=<optimized out>) at ./lv2plugin.cc:531
        self = 0x55558628a600
#6  0x00007ffff618fd5f in CarlaBackend::CarlaPluginLV2::prepareForSave(bool) () from /opt/zrythm-trial-1.0.0.beta.4.8.1/lib/x86_64-linux-gnu/zrythm/carla/libcarla_host-plugin.so
No symbol table info available.
#7  0x00007ffff6159b92 in CarlaBackend::CarlaPlugin::getStateSave(bool) () from /opt/zrythm-trial-1.0.0.beta.4.8.1/lib/x86_64-linux-gnu/zrythm/carla/libcarla_host-plugin.so
No symbol table info available.
#8  0x00007ffff615e56d in CarlaBackend::CarlaPlugin::saveStateToFile(char const*) () from /opt/zrythm-trial-1.0.0.beta.4.8.1/lib/x86_64-linux-gnu/zrythm/carla/libcarla_host-plugin.

So this is trying to save a liquidsfz-0.3.1 plugin in zrythm without sfz file loaded. What happens is that the plugin calls

  map_path->abstract_path (map_path->handle, path.c_str())

and assigns the result to a std::string. In this case path is "". Then carla triggers an assertion failure. It returns a nullptr. Then the plugin assigns a nullptr to a std::string which triggers a crash.

I have released a "fix" for liquidsfz (https://github.com/swesterfeld/liquidsfz/commit/203e1850c92519d70f2374593f21efaaddcb1cb7 and https://github.com/swesterfeld/liquidsfz/commit/a5ac91a0b9a9bbc42fcf0aa76a6bcf2bed464069) which doesn't call map_path->abstract_path any longer if path is empty, mainly to make everything work with Carla. I also will release a change for SpectMorph soon so that it doesn't trigger these cases. But I would recommend to change the code in Carla to return "" and not nullptr in these situations, to avoid similar problems with other plugins.

There are a quite a few different assertions in the code similar to this:

CARLA_SAFE_ASSERT_RETURN(absolute_path != nullptr && absolute_path[0] != '\0', nullptr);

which would be safer if they looked like this

CARLA_SAFE_ASSERT_RETURN(absolute_path != nullptr, nullptr);
CARLA_SAFE_ASSERT_RETURN(absolute_path[0] != '\0', "");

[falkTX]

returning NULL is a valid thing to do though. and the return value needs to be allocated on the heap, returning "" will simply crash when the plugin side tries to free the pointer. an empty string allocation seems pretty useless...

[swesterfeld]

Oops, right, returning "" would not be so nice. Well, it is fixed in my plugins now, so if you want to keep everything like it is thats ok with me.

[falkTX]

can you give a good reason for why the plugin is trying to map an empty path?

[swesterfeld]

Well it typically happens if you have create a new instance of the plugin. Then the filename has to have a value. In liquidsfz this value would be "". Then you want to save it. Then the plugin would ask what the abstract path for "" would be and save that string. I've seen that lilv supports it:

https://github.com/lv2/lilv/blob/d564baafed0863813a87d872f8663134e74228c8/src/state.c#L284

[falkTX]

that seems more like a fix for semi-broken behaviour, though I am ok with adding such a thing just to keep compatibility with other host tools (thanks for the lilv link)

but I still think that trying to map empty path is invalid. at the very least it should be "." to say "current dir"

[falkTX]

err the code style changes in lilv makes it hard to see why/when that code was added :( oh well, will add the "fix" for next release

[swesterfeld]

It may be invalid, which is why I think that triggering an assertion is perfectly fine. But since a plugin crash often means crashing your DAW with all that precious data in it, making it crash is bad for the user.