-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[Swift] Verifier implementation #6373
Conversation
@aardappel Would we want this to land in flatbuffers 2.0? |
c2a993f
to
07c069a
Compare
|
637d8bd
to
869fd2d
Compare
There's no particular reason to make it land before or after 2.0, since it doesn't break the API or anything. Would recommend to land after 2.0 if you're not very sure its solid by the time we're ready to release. |
Then we definitely can keep it after 2.0 lands, since it still not completely documented, nor tested properly. plus, I still haven't gotten to the fuzzing part yet. |
1c2622e
to
50a82c5
Compare
@aardappel it seems that TravisCI is down |
@mustiikhalil yes, not sure what's going on.. |
50a82c5
to
86a73d3
Compare
40eb2af
to
01038f9
Compare
01038f9
to
aed019e
Compare
3632a37
to
35a2ce5
Compare
4854318
to
d3dffd3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the rust verifier is written by @CasperN your input regarding the following comments are highly needed!
@aardappel I've added fuzzing to the swift verifier, and it seems to be working as expected until it comes to the comments in this review. The swift verifier is closely written to match the rust verifier.
and would this be considered as fuzzing? as in we should have it here http://google.github.io/flatbuffers/flatbuffers_support.html
ticked as yes?
24ce799
to
4cb00b9
Compare
75ec4b8
to
fec8f07
Compare
This PR is ready to be merged, implementing the verifier for swift + fuzzing inputs into the verifier to make sure it works as expected. However if it would land before Flatbuffers 2.0, I would love to add a notice of deprecation for |
Well, up to you to decide if it should land before or after. |
Then let's keep it after the next release so we can catch bugs before the |
fec8f07
to
312ade1
Compare
#6636 this should land next :D |
@mustiikhalil go ahead and land it :) |
312ade1
to
0f7d9ee
Compare
Updates test cases on linux Adhere to new protocol naming Adds grpc generated code Adds fuzzing Adds documentation Adds support for string unions Updated fuzzer generated code
0f7d9ee
to
6b61879
Compare
Updates test cases on linux Adhere to new protocol naming Adds fuzzing Adds documentation Adds support for string unions Updated fuzzer generated code
/// - checkAlignment: If alignment check is required to be preformed | ||
/// - Throws: `exceedsMaxSizeAllowed` if capacity of the buffer is more than 2GiB | ||
public init( | ||
buffer: inout ByteBuffer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mustiikhalil , any reason this is an inout parameter? ByteBuffer internally maintains the pointer, so extra copy or not of that pointer should be OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuliu my thought process here is that we don't want to also copy the ByteBuffer
struct too. Trying to make it as fast as possible. I haven't tried it without inout
, however if you see that its faster without the inout please create a PR so we can review it together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it makes some of the wrapping more challenging as I need to pass inout parameters all the way down for ByteBuffer. Do you have a benchmark suite I can run? Definitely can play around with the benchmark and put up a PR for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liuliu Before doing so, can you let me know how are you using the verifier? or how does the code that you currently have will function with the current implementation of the verifier?
Do you have a benchmark suite I can run?
Unfortunately, there isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. I can help to build the benchmark.
I started to add persisted dictionary support in Dflat: https://github.com/liuliu/dflat/blob/unstable/src/sqlite/SQLiteWorkspaceDictionary.swift#L107 Because you can put arbitrary objects encoded there, run verify before decode seems to be a good idea.
Yeah, before doing the benchmark, I need to check a few things, seems adding verify breaks my unit tests somehow ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Dflat isn't going to request data from a network, its safe to assume that the data is always safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bit more back-story I am thinking through (sorting it out), and seeing Verifier
support, thinking it is a good idea. If you can bear with me for the long paragraph:
I am currently working on two new things with Dflat, 1. automatically generate flatbuffers schema from GraphQL queries / schema. 2. having a persisted dictionary support that you can save any FlatBuffersCodable object into a dictionary and read them out.
These two independently, are perfectly safe assuming no data corruption on disk.
For 1., even though there is no guarantee on parameter orders when GraphQL schema changes (FlatBuffer's backward compatibility story requires new field to be appended to the end, old field to never be removed, only deprecated), I can generate "digest" that effectively versioning the GraphQL schema into different sqlite backing tables.
For 2, if you have hand-written flatbuffers schemas, you can always guarantee safety if you save the same object with the same key in the persisted dictionary, and update schema based on the rules.
But if you have generated flatbuffers schema from 1, and try to save that into the persisted dictionary in 2., it can be problematic for end-users. A way to avoid this is to use Verifier, another way is to incorporate the "digest" concept into my wrapper data in 2.
As I said, I am still iterating through how to deal with these, and just want to dip my toe with Verifier as I have other non-open-source project actually depends on network transmitted flatbuffers data :)
This PR would include a swift verifier! Its WIP, however I've managed to make it work on a couple of objects! but I am not sure how it would hold up after the code generator!
Tested objects after code generation:
Closes #6209
Closes #6211