Crash when trying to load nonexistent state

[x42]

While this is an API user error (meanwhile fixed in Ardour), passing a non-existent path to lilv_state_new_from_file, crashes lilv since lilv v0.24.20-10 dd5e851.

zix_canonical_path returns NULL if the path passed to it does not exist. This leads to a crash in serd_node_new_file_uri calling strlen((const char*)path)

Older versions of liblilv are not affected lilv_path_absolute never returned NULL.

==2333==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f9f85b7bc19 bp 0x7ffe29b76d30 sp 0x7ffe29b764d8 T0)
==2333==The signal is caused by a READ memory access.
==2333==Hint: address points to the zero page.
    #0 0x7f9f85b7bc19  (/lib/x86_64-linux-gnu/libc.so.6+0x15dc19) (BuildId: 8a1bf172e710f8ca0c1576912c057b45f90d90d8)
    #1 0x7f9f8d0658dc in __interceptor_strlen ../../../../src/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:459
    #2 0x7f9f8562e31d in serd_node_new_file_uri (/lib/x86_64-linux-gnu/libserd-0.so.0+0xd31d) (BuildId: 62a360419ccf9ac8817f7c4c60b9cfec3cf15e84)
    #3 0x7f9f88299e20 in lilv_state_new_from_file (/lib/x86_64-linux-gnu/liblilv-0.so.0+0xde20) (BuildId: cc7d3e69a11506058f4c11a6c81a8c7c67b1e196)
    #4 0x7f9f8c0be866 in ARDOUR::LV2Plugin::set_state(XMLNode const&, int) ../libs/ardour/lv2_plugin.cc:2320

[drobilla]

While this is an API user error [...]

Technically true, but it sucks that there's no tooling for this. Maybe I should add clang nullability attributes to lilv.h, I've been using them "internally" for ages and it's pretty great, this kind of "oops I passed NULL and shouldn't have" issue more or less goes away entirely (replaced with an obvious compiler warning), for stuff you can compile on clang anyway (and it forces it to be strictly "documented" in any case).

I haven't been messing with the lilv API much since I hope to largely replace it, but come to think of it, this would be better to do before changing anything in the API since it makes migration much easier (annoying that it could just use ZIX_NONNULL and friends, but I'm not sure about publicly exposing the zix dependency yet, what that would mean for versioning etc). I wouldn't be surprised if it catches similar bugs in Ardour and other hosts right now... I'll tinker

[x42]

Ardour passes a valid path, it just happens to not exist on disk [yet].

It's only later, due to zix using realpath when it becomes NULL, so nullability in lilv.h won't help.

[drobilla]

I see. This extra shouldn't be happening already then.

I think it's an annotation "bug" in zix: zix_canonical_path should return ZIX_NULLABLE, not ZIX_ALLOCATED (which effectively means "don't worry/warn about it") but this returns NULL not just on failed allocation, as in this case.

The same is true for zix_create_temporary_directory. I didn't have filesystem errors in my head when I was annotating those I suppose.

[drobilla]

Riiight, I got confused why fixing that caught other similar mistakes in state.c but not this one: the clang, er, "suite" is a bit weird. This is caught by the compiler warning -Wnullable-to-nonnull-conversion (which was suppressed in lilv), but other more tricky ones are caught by the generally smarter clang-tidy (which won't report them if you have the clang diagnostic off).

So! Yep, some NULL handling regressions from the zix change. I'll fix this, and all the others, bringing lilv into the "zero null derefence bugs" party of all its dependencies.

[drobilla]

Fixed in 81bd78d