Mysterious UB in LV2 plugin implementation caused by missing null-terminator in string

Hi. I am trying out writing an LV2 plugin with Rust. (LV2 is an open audio plugin standard.) I realize that there is an existing library for this; I'm doing this as a self-study exercise for working with bindgen and integrating with C stuff.

I've written the bare minimum plugin that just changes the amplitude of a signal up and down. Everything seems to work, but only if I include a call to println! in any of the extern "C" functions.

For more detail, plugins work by providing the following function in a shared library (in C):

const LV2_Descriptor*
lv2_descriptor(uint32_t index)
{
    return index == 0 ? &descriptor : NULL;
}

so that when the plugin host encounters a NULL it knows to stop calling this function. A descriptor is a struct with a URI (a string) and a set of lifecycle callbacks.

The equivalent Rust I'm using is

#[no_mangle]
pub extern "C" fn lv2_descriptor(index: i32) -> *const LV2_Descriptor {
    if index == 0 {
        &d
    } else {
        std::ptr::null()
    }
}

Of course there is more code involved that I've omitted in the body of this post, but the above doesn't work when I test it out with jalv (a terminal based lv2 host), which complains that

lilv_plugin_instantiate(): error: No plugin <http://lv2plug.in/plugins/rust-tom-amp> in <file:///usr/local/lib/lv2/tom-rust-amp.lv2/librust_lv2_ffi_amp.so>

which I take to mean that it was able to call the lv2_descriptors function, but got NULL. To verify this, I changed the body of this function to just

#[no_mangle]
pub extern "C" fn lv2_descriptor(index: i32) -> *const LV2_Descriptor {
    std::ptr::null()
}

which reproduces the same message from jalv. I also tried the opposite, where the function ignores the index and just returns the descriptor every time, which never returned, meaning that this function is being called successfully by the host, but it doesn't find the plugin with the matching URI.

#[no_mangle]
pub extern "C" fn lv2_descriptor(index: i32) -> *const LV2_Descriptor {
    &d
}

Here's what is baffling to me, though. When I added a

println!("The index is: {}", index);

to the top of the body of this function, it printed 0 but also worked (the plugin was launched and I was able to use it). I also tried this in another plugin host (Ardour) and verified similar behavior.

The plugin also works if I place the println! anywhere there is a lifecycle callback. So it seems like the existence of any println! causes Rust to compile it in a way that changes how this function works, but I'm not sure why and how to clean this up so I don't need to include println! unnecessarily.

FWIW my Cargo.toml looks like this

[package]
name = "rust-lv2-ffi-amp"
version = "0.1.0"
edition = "2021"

[lib]
crate-type = ["cdylib"]

[dependencies]

[build-dependencies]
bindgen = "0.70.1"

and this all works the same way for debug and release.

At a minimum, you will have to provide details about this thing...

Nit... Unsigned...

Signed...

In this case, it's unlikely to matter. In general, little details matter.

Thanks. I changed that argument to unsigned (didn't matter as you suggested it might not). The d here looks like this

const d: LV2_Descriptor = LV2_Descriptor {
    URI: AMP_URI.as_ptr() as *const c_char,
    instantiate: Some(instantiate),
    connect_port: Some(connect_port),
    activate: None,
    deactivate: None,
    run: Some(run),
    cleanup: Some(cleanup),
    extension_data: None,
};

I also tried something else which didn't work:

#[no_mangle]
pub extern "C" fn lv2_descriptor(index: u32) -> *const LV2_Descriptor {
    if index == 0 {
        Box::leak(Box::new(LV2_Descriptor {
            URI: AMP_URI.as_ptr() as *const c_char,
            instantiate: Some(instantiate),
            connect_port: Some(connect_port),
            activate: None,
            deactivate: None,
            run: Some(run),
            cleanup: Some(cleanup),
            extension_data: None,
        }))
    } else {
        std::ptr::null()
    }
}

same result.

and here is the type information from bindgen

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct LV2_Descriptor {
    pub URI: *const ::std::os::raw::c_char,
    pub instantiate: ::std::option::Option<
        unsafe extern "C" fn(
            descriptor: *const LV2_Descriptor,
            sample_rate: f64,
            bundle_path: *const ::std::os::raw::c_char,
            features: *const *const LV2_Feature,
        ) -> LV2_Handle,
    >,
    pub connect_port: ::std::option::Option<
        unsafe extern "C" fn(
            instance: LV2_Handle,
            port: u32,
            data_location: *mut ::std::os::raw::c_void,
        ),
    >,
    pub activate: ::std::option::Option<unsafe extern "C" fn(instance: LV2_Handle)>,
    pub run: ::std::option::Option<unsafe extern "C" fn(instance: LV2_Handle, sample_count: u32)>,
    pub deactivate: ::std::option::Option<unsafe extern "C" fn(instance: LV2_Handle)>,
    pub cleanup: ::std::option::Option<unsafe extern "C" fn(instance: LV2_Handle)>,
    pub extension_data: ::std::option::Option<
        unsafe extern "C" fn(uri: *const ::std::os::raw::c_char) -> *const ::std::os::raw::c_void,
    >,
}

AMP_URI is defined as?

const AMP_URI: &str = "http://lv2plug.in/plugins/rust-tom-amp";

const d: LV2_Descriptor is probably not what you want, since that doesn't necessarily have a memory address, and at a guess, you're creating a new instance, returning the address of it, and freeing it before the C code has a chance to use it.

Try making it static d: LV2_Descriptor, which makes it something that's got an address and is not freed. And do similar with AMP_URI - you want it to be a thing in memory, not something the compiler fills in as needed at compile time.

I tried to make it static, but internally it uses types that aren't compatible:

`*const i8` cannot be shared between threads safely
within `LV2_Descriptor`, the trait `Sync` is not implemented for `*const i8`, which is required by `LV2_Descriptor: Sync`
shared static variables must have a type that implements `Sync`

and it's important that I don't change the definitions of those as they come from bindgen.

But also, I had the same thought about memory earlier, and that should've been solved by that Box::leak business (above), I think. The idea being that some memory was created for the thing inside the box that was then leaked (to allow it to be managed/freed by calling code manually). Maybe that reasoning is wrong.

If AMP_URI was const, you'd still be creating a temporary and immediately freeing it, which is not good. const is for things you want the compiler to inline during compilation, static (or leaked heap memory) for things that need an address.

It's true. I was later trying to figure out how to include the AMP_URI as a static, but the compiler doesn't like that. It gave me this, and I'm not sure what it's asking me to do at the end there

referencing statics in constants is unstable
see issue #119618 <https://github.com/rust-lang/rust/issues/119618> for more information
`static` and `const` variables can refer to other `const` variables. A `const` variable, however, cannot refer to a `static` variable.
to fix this, the value can be extracted to a `const` and then used.

still looking into it

But also why would the println! have any effect on this line of thinking? I'm confused about that clue.

AMP_URI should be fine as a const since a string literal always results in a static allocation (the const will hold only a pointer to it). What might be worrying is the fact that this is not guaranteed to be null-terminated, which C might be expect.

Regarding the LV2_Descriptor, a dirty solution might be to do const { &d }, which guarantees it to be promoted to a static. Alternatively you can either implement Sync for LV2_Descriptor or write a wrapper for it that implements Sync, then you should be able to put it in a static manually.

4 Likes

When adding a println! (or printf in C) changes behaviour, that's a sign that you're invoking the dreaded UB somehow, and by perturbing the code, you're changing the specific behaviour of this compilation to be what you want.

It's then a case of looking for possible causes of UB, and trying to eliminate them; in your case, I see const used for things whose addresses matter, and that, to me, suggests a use-after-free.

I would not use const at all (except in the pointer casts), and have all the variables be static for now. The thing the compiler is complaining about is that you have a const var: Type that references a static, and you want to change that to a static, not a const, too, to reduce the UB surface.

2 Likes

Thanks! I noticed via xxd that the string is stored literally in the .so, which was not null-terminated. Manually null-terminating it did the trick!

1 Like

Appreciate the insight on that. I'm coming from higher-level languages so some of the thought process around UB isn't etched into my brain yet.

1 Like

Rather than using a String, use a CString; this is a null-terminated C-style string by definition

Since this is a literal OP could also use C string literals (i.e.c"put your string here"), available since Rust 1.77.

5 Likes

This feels better to me, because CString can't be used with const or static and because of that you end up having to Box::leak both the URI and the enclosing LV2_Descriptor in the function body in order for the address to point to something still alive. It seemed clearer and more maintainable to just put a \0 at the end, but the built-in C string literal syntax is arguably better.

1 Like

Unfortunately no, that's not quite correct. This does ensure that it is &'static, but unlike an actual static item with a single address, the const block version can still be duplicated and exist at multiple different addresses. “Promoted to static storage” is accurate; “promoted to a static” is misleading at best. Saying “promoted to 'static” is also accurate since that is referring to the lifetime directly.

I point this out since the stable address provided by static items was discussed as potentially relevant.

2 Likes

Would a run-once thing that returns a pointer to a forgotten structure be a good solution?