Multiple "custom extension" callbacks with same signature share indices

[nicolaskagami]

When using the function add_custom_ext the callbacks are stored using indices that depend solely on their function signature, as implemented on cached_ex_index. More specifically, a TypeId of the signature is used to generate the key.

This means that two different callbacks with the same signature will overwrite each other.
The key for the index should depend on its identifier (a.k.a. "extension type") as there should be a one-to-one relation between an extension type and its callbacks.

[sfackler]

TypeId doesn't soley depend on the function signature. Two closures with the same signature will have distinct type IDs: https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=2cbed57c6704def2b82e7ef890c83c16

[nicolaskagami]

TypeId doesn't soley depend on the function signature. Two closures with the same signature will have distinct type IDs: https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=2cbed57c6704def2b82e7ef890c83c16

Hmm. I'm not sure why they are not the same in the case you provided. However, I have another snippet that illustrates the point more clearly: https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=b7409b138b7e33cacf7c630753ac49f2

Notice how they are not even "the same closure" (i.e. they have different instructions) but they still have the same TypeId when compared in this way.

I have experienced this problem first-hand when dealing with custom extensions with rust-openssl.

[sfackler]

If you coerce them to function pointers they'll have the same type id, sure.

[nicolaskagami]

I added the following test to the PR I opened:

#[test]
#[cfg(ossl111)]
fn custom_extensions_with_same_callback_signature() {
    static FOUND_EXTENSION_1: AtomicBool = AtomicBool::new(false);
    static FOUND_EXTENSION_2: AtomicBool = AtomicBool::new(false);

    fn insert_custom_extension(builder: &mut SslContextBuilder, ext_type: u16, data: Vec<u8>) {
        builder
            .add_custom_ext(
                ext_type,
                ExtensionContext::CLIENT_HELLO,
                move |_, _, _| Ok(Some(data.clone())),
                |_, _, _, _| Ok(()),
            )
            .expect("Failed to add custom extension");
    }

    let mut server = Server::builder();
    server
        .ctx()
        .add_custom_ext(
            12345,
            ExtensionContext::CLIENT_HELLO,
            |_, _, _| -> Result<Option<&'static [u8]>, _> { unreachable!() },
            move |_, _, data, _| {
                FOUND_EXTENSION_1.store(data == b"some data".to_vec(), Ordering::SeqCst);
                Ok(())
            },
        )
        .unwrap();
    server
        .ctx()
        .add_custom_ext(
            23456,
            ExtensionContext::CLIENT_HELLO,
            |_, _, _| -> Result<Option<&'static [u8]>, _> { unreachable!() },
            move |_, _, data, _| {
                FOUND_EXTENSION_2.store(data == b"some other data".to_vec(), Ordering::SeqCst);
                Ok(())
            },
        )
        .unwrap();

    let server = server.build();
    let mut client = server.client();
    insert_custom_extension(client.ctx(), 12345, b"some data".to_vec());
    insert_custom_extension(client.ctx(), 23456, b"some other data".to_vec());
    client.connect();

    assert!(FOUND_EXTENSION_1.load(Ordering::SeqCst));
    assert!(FOUND_EXTENSION_2.load(Ordering::SeqCst));
}

Don't you think this use case deserves to be addressed?