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

[Rust] get_root_as_*** design issue (panic on errors) #5000

Closed
rumatoest opened this issue Oct 15, 2018 · 10 comments

Comments

@rumatoest
Copy link

commented Oct 15, 2018

Latest flatbuffers version generates next code to read root structures

pub fn get_root_as_service_response<'a>(buf: &'a [u8]) -> ServiceResponse<'a> {
  flatbuffers::get_root::<ServiceResponse<'a>>(buf)
}

But wait, If something bad happens this code panics. WHAT! (O_o)
Do you know that rust has Result for such purposes and function signature MUST be like this

pub fn get_root_as_service_response<'a>(buf: &'a [u8]) -> Result<ServiceResponse<'a>, Error> {
  flatbuffers::get_root::<ServiceResponse<'a>>(buf)
}

How can you ever ignore such thing as Result?

@aardappel

This comment has been minimized.

Copy link
Collaborator

commented Oct 15, 2018

In what situation does it panic? array out of bounds? that is probably intentional as we wouldn't want to have user code check that upon every access. I believe a verifier is the reasonable alternative when corrupt data has to be dealt with gracefully.

@rumatoest

This comment has been minimized.

Copy link
Author

commented Oct 16, 2018

In what situation does it panic?

Invalid input buf: [u8] that can not be read

I believe a verifier is the reasonable alternative when corrupt data has to be dealt with gracefully.

But in Rust graceful way to deal with error is to return Result.

I is better to make get_root_as_... function to always return Result
And for other cases create another method like get_root_as_..._unwraped which name should give a hint about possible panic, because this is what you can get if you call unwrap() functions.

@aardappel

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2018

It's not just get_root_as, every accessor would need to be returning Result. We don't do this in any of the language APIs, the result of accessing a corrupt buffer is an index out of bounds exception in most languages, or simply undefined behavior in C/C++. In no language is this error propagated as part of the API, which would be very expensive and clumsy. The verifier in C++ means you can have a check for a corrupt buffer in a single location, such that during actual access such errors are guaranteed not to occur. It would make a lot of sense for Rust to follow the same model, since it is also performance oriented.

@rumatoest

This comment has been minimized.

Copy link
Author

commented Oct 18, 2018

It would make a lot of sense for Rust to follow the same model
Which is incompatible with Rust approach for handling errors.

since it is also performance oriented.
I doubt that.
BTW in my pet project I can't see any performance in Rust implementation at all.

@aardappel

This comment has been minimized.

Copy link
Collaborator

commented Oct 18, 2018

Languages have different classes of errors. Does array indexing in Rust return a Result ? Does the division operator?

@fbenkstein

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2018

I think in cases where Rust takes values than can come from third parties and converts to something else Result is usually returned. get_root_as_... is more like from_utf8 which does return Result. To keep the behavior between languages consistent, maybe the contract of the flatbuffers Rust library could be that panics do not occur if verify_ has been called?

@aardappel

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2018

the contract of the flatbuffers Rust library could be that panics do not occur if verify_ has been called?

Yes, that makes sense to me. That's the contract you get in C++ (no segfaults in that case).

@rw

This comment has been minimized.

Copy link
Collaborator

commented Oct 22, 2018

@rumatoest @fbenkstein The current semantics of the language ports, Rust included, is that the code is allowed to assume that the underlying flatbuffer is valid. If the data is not valid, then the different ports have different ways of dealing with this (generally they panic on out-of-bounds access, except in Rust/C/C++ where they may perform undefined behavior during a reinterpret_cast-type of call).

The most robust solution to handle untrusted data is to run the Verifier (currently only in C++) to validate the correctness of a flatbuffer. We're working on a Verifier for Rust so that we can have the same developer experience in both of these languages.

@rw

This comment has been minimized.

Copy link
Collaborator

commented Oct 24, 2018

@aardappel @rumatoest @fbenkstein I'd like to close this as non-constructive. Maybe we need better docs on these design decisions?

@aardappel

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

@rw I think it is important to clarify that going out of bounds on a FlatBuffer should be treated similarly to how the language treats going out of bounds of an array, for a buffer that is corrupt and has not been verified.

But yes, we can close it.

@aardappel aardappel closed this Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.