search

dbus2 / zbus-old

Rust D-Bus crate.
https://gitlab.freedesktop.org/dbus/zbus
Other
49 stars 13 forks source link

stack overflow with wrongly derived deserialize implementation #174

Closed zeenix closed 1 year ago

zeenix commented 3 years ago

In GitLab by @jneem on May 10, 2021, 22:20

I was trying to follow along with @bilelmoussaoui's excellent post, for making a file chooser dialog, but I got stuck deserializing the response.

The short version is that I was mistakenly using #[derive(serde::Deserialize)] when I should have been using #[derive(zvariant_derive::DeserializeDict)]. So 100% my fault, but it would be nice if there were a more helpful error than a stack overflow.

The longer version is that my structs that I was trying to deserialize into were:

#[derive(serde::Deserialize, Debug)]
struct ResponseResult(u32, Response);

#[derive(serde::Deserialize, Debug)]
struct Response {
    uris: Vec<String>,
    choices: Option<Vec<(String, String)>>,
}

And here is a small, cyclic, portion of the backtrace:

#18211 0x000055555565457f in zvariant::de::deserialize_any<byteorder::LittleEndian,&mut zvariant::dbus::de::Deserializer<byteorder::LittleEndian>,serde::de::ignored_any::IgnoredAny> (de=0x7fffffff9e10, next_char=118 'v', visitor=...) at /home/jneeman/.cargo/git/checkouts/zbus-e0109d69954ffaa9/cf489ca/zvariant/src/de.rs:380
#18212 0x00005555555e6f7c in zvariant::dbus::de::{{impl}}::deserialize_any<byteorder::LittleEndian,serde::de::ignored_any::IgnoredAny> (self=0x7fffffff9e10, visitor=...) at /home/jneeman/.cargo/git/checkouts/zbus-e0109d69954ffaa9/cf489ca/zvariant/src/dbus/de.rs:81
#18213 0x00005555555f7ce6 in zvariant::dbus::de::{{impl}}::deserialize_ignored_any<byteorder::LittleEndian,serde::de::ignored_any::IgnoredAny> (self=0x7fffffff9e10, visitor=...) at /home/jneeman/.cargo/git/checkouts/zbus-e0109d69954ffaa9/cf489ca/zvariant/src/dbus/de.rs:64
#18214 0x0000555555617b97 in serde::de::ignored_any::{{impl}}::deserialize<&mut zvariant::dbus::de::Deserializer<byteorder::LittleEndian>> (deserializer=0x7fffffff9e10) at /home/jneeman/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.125/src/de/ignored_any.rs:241
#18215 0x00005555556e3246 in serde::de::{{impl}}::deserialize<serde::de::ignored_any::IgnoredAny,&mut zvariant::dbus::de::Deserializer<byteorder::LittleEndian>> (self=..., deserializer=0x7fffffff9e10) at /home/jneeman/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.125/src/de/mod.rs:785
#18216 0x00005555555cae2d in zvariant::dbus::de::{{impl}}::next_element_seed<byteorder::LittleEndian,core::marker::PhantomData<serde::de::ignored_any::IgnoredAny>> (self=0x7fffffff73c0, seed=...) at /home/jneeman/.cargo/git/checkouts/zbus-e0109d69954ffaa9/cf489ca/zvariant/src/dbus/de.rs:536
#18217 0x00005555555c5067 in serde::de::SeqAccess::next_element<zvariant::dbus::de::ValueDeserializer<byteorder::LittleEndian>,serde::de::ignored_any::IgnoredAny> (self=0x7fffffff73c0) at /home/jneeman/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.125/src/de/mod.rs:1707
#18218 0x00005555556176f9 in serde::de::ignored_any::{{impl}}::visit_seq<zvariant::dbus::de::ValueDeserializer<byteorder::LittleEndian>> (self=..., seq=...) at /home/jneeman/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.125/src/de/ignored_any.rs:201
#18219 0x00005555555ec448 in zvariant::dbus::de::{{impl}}::deserialize_seq<byteorder::LittleEndian,serde::de::ignored_any::IgnoredAny> (self=0x7fffffff9e10, visitor=...) at /home/jneeman/.cargo/git/checkouts/zbus-e0109d69954ffaa9/cf489ca/zvariant/src/dbus/de.rs:258
#18220 0x000055555565457f in zvariant::de::deserialize_any<byteorder::LittleEndian,&mut zvariant::dbus::de::Deserializer<byteorder::LittleEndian>,serde::de::ignored_any::IgnoredAny> (de=0x7fffffff9e10, next_char=118 'v', visitor=...) at /home/jneeman/.cargo/git/checkouts/zbus-e0109d69954ffaa9/cf489ca/zvariant/src/de.rs:380

The full code follows, and works with zbus 2.0.0-beta.3 (or git main)

use std::collections::HashMap;

use zbus::{dbus_proxy, fdo, Connection};
use zvariant::{OwnedObjectPath, OwnedValue, Value};

#[derive(serde::Deserialize, Debug)]
// no stack overflow if I use this instead.
// struct ResponseResult(u32, HashMap<String, OwnedValue>);
struct ResponseResult(u32, Response);

#[derive(serde::Deserialize, Debug)]
struct Response {
    uris: Vec<String>,
    choices: Option<Vec<(String, String)>>,
}

impl zvariant::Type for Response {
    fn signature() -> zvariant::Signature<'static> {
        <HashMap<&str, OwnedValue>>::signature()
    }
}

impl zvariant::Type for ResponseResult {
    fn signature() -> zvariant::Signature<'static> {
        <(u32, Response)>::signature()
    }
}

#[dbus_proxy(
    interface = "org.freedesktop.portal.FileChooser",
    default_service = "org.freedesktop.portal.Desktop",
    default_path = "/org/freedesktop/portal/desktop"
)]
trait FileChooser {
    fn open_file(
        &self,
        parent_window: &str,
        title: &str,
        options: HashMap<&str, &Value<'_>>,
    ) -> fdo::Result<OwnedObjectPath>;
}

fn main() -> fdo::Result<()> {
    let connection = Connection::new_session()?;
    let proxy = FileChooserProxy::new(&connection);
    let reply = proxy.open_file("", "Title", HashMap::new())?;
    dbg!(reply);

    loop {
        let msg = connection.receive_message()?;
        let msg_header = msg.header()?;
        if msg_header.message_type()? == zbus::MessageType::Signal
            && msg_header.member()? == Some("Response")
        {
            dbg!(msg.body_signature()?);
            dbg!(msg.body::<ResponseResult>()?);
            break;
        }
    }
    Ok(())
}
zeenix commented 3 years ago

In GitLab by @bilelmoussaoui on May 10, 2021, 22:26

Any reason for not using ashpd directly for that? we already migrated to zbus (from the master branch currently) which handles signals properly

zeenix commented 3 years ago

In GitLab by @jneem on May 11, 2021, 24:38

Mostly just because I wanted to understand what was going on underneath the abstractions. (Also, ashpd master isn't building for me right now, apparently because of API changes in zbus)

zeenix commented 3 years ago

In GitLab by @bilelmoussaoui on May 11, 2021, 24:52

It should compile just fine now :-)

zeenix commented 3 years ago

In GitLab by @jhartzell42 on Jul 9, 2021, 16:39

I am hitting the same stack overflow trying to use serde-value to implement adjacency enums in a DeserializeDict context. It looks like: deserialize_any on a variant leads to saying it's a sequence but when it comes time to parse the first element, also using deserialize_any, it is still a variant so it says it's a sequence...

#56 0x0000561ba00a136c in <&mut zvariant::dbus::de::Deserializer<B> as serde::de::Deserializer>::deserialize_any (self=0x7fffd74523a0, visitor=...)
    at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/zvariant-2.7.0/src/dbus/de.rs:84
#57 0x0000561ba004f8b7 in serde_value::de::<impl serde::de::Deserialize for serde_value::Value>::deserialize (d=0x7fffd74523a0)
    at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-value-0.7.0/src/de.rs:296
#58 0x0000561ba008f8a6 in <core::marker::PhantomData<T> as serde::de::DeserializeSeed>::deserialize (self=..., deserializer=0x7fffd74523a0)
    at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.126/src/de/mod.rs:785
#59 0x0000561ba009f694 in <zvariant::dbus::de::ValueDeserializer<B> as serde::de::SeqAccess>::next_element_seed (self=0x7fffd6c654d0, seed=...)
    at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/zvariant-2.7.0/src/dbus/de.rs:539
#60 0x0000561ba009f337 in serde::de::SeqAccess::next_element (self=0x7fffd6c654d0) at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-1.0.126/src/de/mod.rs:1707
#61 0x0000561ba0057b7d in <serde_value::de::ValueVisitor as serde::de::Visitor>::visit_seq (self=..., visitor=...)
    at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/serde-value-0.7.0/src/de.rs:271
#62 0x0000561ba00a2088 in <&mut zvariant::dbus::de::Deserializer<B> as serde::de::Deserializer>::deserialize_seq (self=0x7fffd74523a0, visitor=...)
    at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/zvariant-2.7.0/src/dbus/de.rs:261
#63 0x0000561ba00487ef in zvariant::de::deserialize_any (de=0x7fffd74523a0, next_char=118 'v', visitor=...)
    at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/zvariant-2.7.0/src/de.rs:383
#64 0x0000561ba00a136c in <&mut zvariant::dbus::de::Deserializer<B> as serde::de::Deserializer>::deserialize_any (self=0x7fffd74523a0, visitor=...)
    at /home/jim/.cargo/registry/src/github.com-1ecc6299db9ec823/zvariant-2.7.0/src/dbus/de.rs:84
zeenix commented 3 years ago

In GitLab by @jhartzell42 on Jul 9, 2021, 17:53

(To be clear, this isn't zbus's ValueVisitor in this context, but rather the one from serde_value).

zeenix commented 3 years ago

I finally started looking into this but I couldn't convert your sample into a testcase:

        #[derive(Serialize, Deserialize, Debug)]
        struct ResponseResult(u32, Response);

        impl zvariant::Type for ResponseResult {
            fn signature() -> zvariant::Signature<'static> {
                <(u32, Response)>::signature()
            }
        }

        #[derive(Serialize, Deserialize, Debug)]
        struct Response {
            uris: Vec<String>,
            choices: Option<Vec<(String, String)>>,
        }

        impl zvariant::Type for Response {
            fn signature() -> zvariant::Signature<'static> {
                <HashMap<&str, OwnedValue>>::signature()
            }
        }

        let ctxt = Context::<LE>::new_dbus(0);
        let res = ResponseResult(7, Response {
            uris: vec!["hello".into()],
            choices: Some(vec![("bye".into(), "now".into())]),
        });
        let encoded = to_bytes(ctxt, &res).unwrap();
        from_slice::<_, ResponseResult>(&encoded, ctxt).unwrap();

I just get an error as I should be getting since the type signature doens't match:

thread 'tests::issue_164' panicked at 'called `Result::unwrap()` on an `Err` value: Message("invalid type: character `a`, expected `(` or `{`")', zvariant/src/lib.rs:1716:44

P.S. Option is not encodable with D-Bus btw. That's why I recently added Optional type.

zeenix commented 3 years ago

In GitLab by @jneem on Aug 12, 2021, 18:24

Thanks for looking into this! I can still reproduce the issue with 2.0.0-beta.6; I made a gist including the Cargo.toml for convenience. I haven't yet had time, but I'll see if I can convert this into a shorter test case.

zeenix commented 2 years ago

@jneem ping! Any luck?

zeenix commented 2 years ago

In GitLab by @jneem on Dec 19, 2021, 18:33

Sorry for the delay! I've managed to reduce the test case to something using on zvariant. I checked that this still causes a stack overflow on zvariant 3.0.

use byteorder::LE;
use std::collections::HashMap;

use zvariant::{EncodingContext, OwnedValue};

#[derive(serde::Deserialize, Debug)]
// no stack overflow if I use this instead.
// struct ResponseResult(u32, HashMap<String, OwnedValue>);
struct ResponseResult(u32, Response);

#[derive(serde::Deserialize, Debug)]
struct Response {
    uris: Vec<String>,
    choices: Option<Vec<(String, String)>>,
}

impl zvariant::Type for Response {
    fn signature() -> zvariant::Signature<'static> {
        <HashMap<&str, OwnedValue>>::signature()
    }
}

impl zvariant::Type for ResponseResult {
    fn signature() -> zvariant::Signature<'static> {
        <(u32, Response)>::signature()
    }
}

fn main() {
    // This is the message I get from dbus if I open a file dialog and then hit "Cancel"
    let msg: Vec<u8> = vec![
        108, 4, 1, 1, 28, 0, 0, 0, 57, 3, 0, 0, 165, 0, 0, 0, 1, 1, 111, 0, 47, 0, 0, 0, 47, 111,
        114, 103, 47, 102, 114, 101, 101, 100, 101, 115, 107, 116, 111, 112, 47, 112, 111, 114,
        116, 97, 108, 47, 100, 101, 115, 107, 116, 111, 112, 47, 114, 101, 113, 117, 101, 115, 116,
        47, 49, 95, 49, 51, 50, 47, 116, 0, 2, 1, 115, 0, 30, 0, 0, 0, 111, 114, 103, 46, 102, 114,
        101, 101, 100, 101, 115, 107, 116, 111, 112, 46, 112, 111, 114, 116, 97, 108, 46, 82, 101,
        113, 117, 101, 115, 116, 0, 0, 6, 1, 115, 0, 6, 0, 0, 0, 58, 49, 46, 49, 51, 50, 0, 0, 8,
        1, 103, 0, 6, 117, 97, 123, 115, 118, 125, 0, 0, 0, 0, 0, 3, 1, 115, 0, 8, 0, 0, 0, 82,
        101, 115, 112, 111, 110, 115, 101, 0, 0, 0, 0, 0, 0, 0, 0, 7, 1, 115, 0, 4, 0, 0, 0, 58,
        49, 46, 57, 0, 0, 0, 0, 1, 0, 0, 0, 20, 0, 0, 0, 4, 0, 0, 0, 117, 114, 105, 115, 0, 2, 97,
        115, 0, 0, 0, 0, 0, 0, 0, 0,
    ];

    let ctxt = EncodingContext::<LE>::new_dbus(0);
    let body_offset = 184;
    let _: ResponseResult = zvariant::from_slice(&msg[body_offset..], ctxt).unwrap();
}
zeenix commented 2 years ago

Sorry for the delay!

Np. :)

I've managed to reduce the test case to something using on zvariant.

Awesome! Thanks so much. I'll try and fix it this week.

I checked that this still causes a stack overflow on zvariant 3.0.

Thanks. I wasn't expecting 3.0 to have fixed it cause we didn't have any relevant changes.

zeenix commented 2 years ago

@jneem There is a major issues with the code sample: You are telling zvariant (with your Type impl of Response that it's a dictionary type but then you're using a struct with the usual struct deserialization of serde. I know for JSON (and probably other formats), struct and hashmap are interchangeable formats as the encoded format is the same for both, but that's not true for D-Bus.

For using structs as Dict type, you want to make use of TypeDict and DeserializeDict here. The following code works:

use byteorder::LE;
use zvariant::{EncodingContext, TypeDict, DeserializeDict, Type};

#[derive(serde::Deserialize, Type, Debug)]
struct ResponseResult(u32, Response);

#[derive(TypeDict, DeserializeDict, Debug)]
struct Response {
    uris: Vec<String>,
    choices: Option<Vec<(String, String)>>,
}

// main function remains unchanged.

Having said that, ideally this shouldn't break zvariant but this might be a much harder problem to debug and solve than I first thought (if possible even since we're limited by serde API/mechanisms).

zeenix commented 2 years ago

@jneem So sorry, I re-read the desciption and title of this issue and rmembered that you're well aware that the code is wrong. It's been a long while when this was first filed and I had my 3rd vaccine today so not operating on full batteries here. :)

Anyway. Hopefully I can finally debug this issue soon and hopefully fix it. Just hope it's not super hard.

zeenix commented 2 years ago

In GitLab by @jneem on Dec 21, 2021, 06:16

No worries :) I had actually forgotten it myself, to be honest...

zeenix commented 1 year ago

Pretty sure this was fixed with !565. We even added relevant fuzzing to our CI, which gives us even more confidence. But just to be sure, I tried the reproducer code above and we don't get an overflow against latest zvariant release.