Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

generates broken binary? #130

Open
raven-stas opened this issue Oct 23, 2018 · 12 comments
Open

generates broken binary? #130

raven-stas opened this issue Oct 23, 2018 · 12 comments

Comments

@raven-stas
Copy link

raven-stas commented Oct 23, 2018

hi, trying to generate a simple message with 2 strings i.e.

syntax = "proto2";

message HelloRequest {
   required string one = 1;
   required string two = 2;
}

and when both args are single spaces, then the binary data generated is [6, 10, 1, 32, 18, 1, 32].

But when using other platforms (tried 2 other different ones), then the generated binary is [10, 1, 32, 18, 1, 32].

Hence when quick-protobuf tries to parse the message generated by the other platforms it simply crashes in panic. It's possible to use the lib when both client and server use it but it's unusable with other platforms.

What am I doing wrong? Any idea what's going on here?

@tafia
Copy link
Owner

tafia commented Oct 24, 2018

I believe you're writing the message length first with quick-protobuf but not with the other libs (you are calling serialize_into_vec or Writer::write_message instead of MessageWrite::write_message directly).
Can you show me the code you use to generate the message?

@raven-stas
Copy link
Author

It's when trying to read the message generated on the other platform. So I was getting an error when reading it and ended up printing out what was generated there which is [10, 1, 32, 18, 1, 32]. So I wrote a short test:

let mut bytes: Vec<u8> = vec![ 10, 1, 32, 18, 1, 32 ];
let mut reader = BytesReader::from_bytes(&bytes);
let msg = reader.read_message::<hello::HelloRequest>(&bytes).expect("can't read!");

This test crashes. I printed out what's generated with the quick-protobuf and it's [6, 10, 1, 32, 18, 1, 32].

So if I give this test [6, 10, 1, 32, 18, 1, 32] as input, all works. If I give what's generated with the other 2 different libs i.e. [10, 1, 32, 18, 1, 32] then it crashes.

If you still need the way I do the write, I can post it too but little later, well if it makes sense.

@tafia
Copy link
Owner

tafia commented Oct 24, 2018

Can you show how you read the message?

@raven-stas
Copy link
Author

Something like:

let req_slice = std::slice::from_raw_parts((req_packet.data as (* const u8)), req_packet.dataLength);
println!("got here.... {:#?}", req_slice);
let req_result: Result<hello::HelloRequest, Error> = deserialize_from_slice(req_slice);

writing from the quick-protobuf client was something like:

let hello_req = hello::HelloRequest {
                        greeting: Cow::Borrowed(" "),
                        name: Cow::Borrowed(" ")
};
let payload = serialize_into_vec(&hello_req).expect("failed to write request!");
println!("payload {:#?}", payload);

I don't think there's something wrong with the type casting - I've switched currently to the rust-protobuf lib which sends ang receives data with the same set up.

@therealprof
Copy link
Contributor

@tafia I ran into the same problem earlier today. I find it a bit unconvincing and awkward that the Writer essentially cannot be used as such since it prefixes the message with a length and as such is not idempotent.

I figured out by myself that I had to do:

msg.write_message(&mut writer);

instead of

writer.write_message(&msg);

and was just about to add a ticket about this when I stumbled across this one.

Is there any reason for such odd behaviour? Can we get a new method that writes a message without a len or at the very minimum get a big fat warning about this unexpected API behaviour?

@mullr
Copy link
Contributor

mullr commented Sep 23, 2019

This needs to at least be called out in the documentation; this is terrifically confusing.

@dbcfd
Copy link

dbcfd commented Nov 16, 2019

Same, I ended up just having to do a roundtrip test to figure it out

pub fn serialize(msg: &Foo) -> Result<Vec<u8>, Error> {
    let mut v = Vec::with_capacity(msg.get_size());
    let mut writer = Writer::new(&mut v);
    msg.write_message(&mut writer).map_err(Error::Protobuf)?;
    Ok(v)
}

pub fn deserialize(bytes: Vec<u8>) -> Result<FooOwned, Error> {
    FooOwned::try_from(bytes).map_err(Error::Protobuf)
}

@nerdrew
Copy link
Collaborator

nerdrew commented Nov 21, 2019

I'll admit I was also confused by this. In fact, I need to lookup my code just about every time I need to write to remember which one to use. Thoughts on documentation improvements? Should we put inline examples on both readers / writers on how to use them?

@maxmcd
Copy link

maxmcd commented Jan 7, 2020

I was also confused by this. Would be great to see extensive use of examples in the documentation as they would then also be subject to testing.

Would be happy to PR some of that as well if I get some extra cycles.

Edit:

For some reason I couldn't get those serialize/deserialize examples to work, mine are:

use quick_protobuf::{BytesReader, MessageRead, MessageWrite, Writer};
pub fn serialize(msg: &Http) -> Result<Vec<u8>, Error> {
    let mut v = Vec::with_capacity(msg.get_size());
    let mut writer = Writer::new(&mut v);
    msg.write_message(&mut writer)?;
    Ok(v)
}

pub fn deserialize(bytes: &Vec<u8>) -> Result<Http, Error> {
    let mut reader = BytesReader::from_bytes(&bytes);
    let out = Http::from_reader(&mut reader, &bytes)?;
    Ok(out)
}

@walfie
Copy link

walfie commented Feb 13, 2020

I also just ran into this -- it wasn't obvious that the length prefixing was happening until I compared the output with the output of a different library, and then finding this issue.

@benma
Copy link

benma commented Jun 20, 2020

👍 this was supremely confusing. I could not get compatibility with the protobuf written/read by other libraries until I figured out that the messages were framed. I don't expect them to be framed, as the libraries of other languages do not frame them either.

@ahenshaw
Copy link

Spent an hour debugging this in my own code. Can an alternative version of serialize_into_vec that doesn't prefix with the length be created?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants