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

Add packet_spec packet mode to packet parser #2079

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Vagabond
Copy link
Contributor

@Vagabond Vagabond commented Jan 7, 2019

packet_spec is a way to parse simple arbitrary binary packet protocols
where the length field is not the first field. Packet specs consist of a
list of [ 'u8' | 'u16' | u16le' | 'u32' | 'u32le' | 'varint' ]. The last
field listed is the length of the payload. Fields before the final field
contribute to the header length. Fixed length footers can be described
with the {packet_spec_extra_bytes, non_neg_integer()} option.

Why do we need this new feature?

Erlang's built in packet parser currently supports big-endian length-prefixed packets, ASN.1 BER packets, CORBA packets, SUN RPC packets, HTTP packets, etc. However there are many packet formats that almost work with Erlang's existing packet parser. For example many packets have a fixed length leader before the length field, or have a little-endian length field. This feature is an attempt to provide a simple way to delegate packet parsing of simple binary packets that are not simply big-endian length prefixed to the packet parser.

Risks or uncertain artifacts?

I could use guidance on the best way to implement some of this, especially how the packet_spec struct is initialized and passed around and how the prim_inet changes should look. I believe the feature itself is not a risk because it does not allow unbounded or infinite packets and does not consume unbounded memory.

How did you solve it?

As can be seen from the patch, a new packet mode and 2 new packet options have been added. I tried to avoid any dynamic allocation and do everything on the stack, although I am not sure this is correct as it is an optional feature. Additionally I added special encoding for the packet_spec in prim_inet and I'm not sure that is the correct approach.

Please let me know if this feature is useful. I wrote a blog post detailing the motivations behind it in more detail here: http://vagabond.github.io/programming/2018/12/30/notes-on-extending-the-erlang-packet-parser

packet_spec is a way to parse simple arbitrary binary packet protocols
where the length field is not the first field. Packet specs consist of a
list of [ 'u8' | 'u16' | u16le' | 'u32' | 'u32le' | 'varint' ]. The last
field listed is the length of the payload. Fields before the final field
contribute to the header length. Fixed length footers can be described
with the `{packet_spec_extra_bytes, non_neg_integer()}` option.
@CLAassistant
Copy link

CLAassistant commented Jan 7, 2019

CLA assistant check
All committers have signed the CLA.

@RoadRunnr
Copy link
Contributor

I have always wondered whether the special packet processing it really worth the effort. The existing code is there for compatibility, but do we really need to add more of this when the same problem can be solved with a few lines of pure Erlang code in the decoder/encoder for a given protocol?

Also, what about protocols:

  • that specify header and packet lengths in multiple of 4 or 8?
  • where the length field is the fraction of bytes, e.g. 12bits or 5 bits ?

Sample of one of those: https://tools.ietf.org/html/rfc5415#section-4.3

@benoitc
Copy link

benoitc commented Jan 7, 2019

@RoadRunnr one advantages is that the data are buffered inside the c code until the size is achievrd. So it doesn’t require to keep a state in erlang which makes the API simpler though.

@rickard-green rickard-green added the team:PS Assigned to OTP team PS label Jan 7, 2019
@Vagabond
Copy link
Contributor Author

Vagabond commented Jan 7, 2019

I find the {packet, N} modes very useful (maybe not so much the other ones) because I can simply get a message when a packet has arrived. I don't need to read on the socket, accumulate a buffer, etc.

I don't know about packed length fields, obviously this is not a completely general purpose way to specify packets. I'd love to hear any suggestions on how to express something like that in a reasonable way. Ideally you'd pass a binary match or something like a BPF program down that could match more oddball protocols, but that was beyond my abilities.

I'm also not sure we need to support every binary protocol (the current list of them certainly is very opinionated), this is just a way to make a bunch more (any byte-aligned, multiple of 8 bit length field) protocols work.

@garazdawi
Copy link
Contributor

Since we are in the process of re-writing the inet driver into a NIF, my plan for these types of things has been to allow for that implementation to allow passing a fun as the packet option. That way inet can support any protocol and not just what we happen to have implemented in C.

That being said, I'm not against us providing the functionality in the PR before then and then reimplementing it using Erlang once the inet NIF is ready.

@benoitc
Copy link

benoitc commented Jan 8, 2019

@garazdawi interresting. how would it work? would the fun pass bytes / bytes or thw buffer collected and reinject some part in it?

@garazdawi
Copy link
Contributor

I haven't thought too much about it, but a similar/identical interface to erlang:decode_packet/3 should be a good starting place.

@benoitc
Copy link

benoitc commented Jan 8, 2019

@garazdawi ok I was askiing since you can't handle a function at the nif level, so i'm curious about the api. It could be useful for a lot of stuff actually :)

@IngelaAndin
Copy link
Contributor

I am not sure how much value this adds in comparison to maintenance. Such packing is easy to handle yourself with the bitsyntax, which did not exist when the first packet options where implemented. When we implement TLS we did implemented a "TLS-record" packet option that we ended up not using, as the context switch to the inet driver was not worth it, it was more effective to handle it in Erlang.

@garazdawi
Copy link
Contributor

ok I was askiing since you can't handle a function at the nif level, so i'm curious about the api. It could be useful for a lot of stuff actually :)

@benoitc just return from the nif and call the fun.

@Vagabond
Copy link
Contributor Author

Vagabond commented Jan 9, 2019

The NIF/fun approach sounds great! When is the inet rewrite planned to land?

@IngelaAndin
Copy link
Contributor

As usual we can not promise but inet NIF rewrite is planned to have something experimental for OTP 22.
But inet driver will have to be phased out gradually.

@IngelaAndin IngelaAndin removed team:PS Assigned to OTP team PS labels Jan 22, 2019
@proger
Copy link

proger commented Jan 25, 2019

Super excited to hopefully see this feature land even before the inet rewrite! I found it is also related to EEP 34: Extended basic packet options for decode_packet which is almost nine years old.

@IngelaAndin IngelaAndin added the team:PO Assigned to OTP team PO label Oct 11, 2023
@rickard-green rickard-green added team:PS Assigned to OTP team PS and removed team:PO Assigned to OTP team PO labels Oct 11, 2023
@IngelaAndin IngelaAndin added the team:PO Assigned to OTP team PO label Oct 24, 2023
@IngelaAndin IngelaAndin added the stalled waiting for input by the Erlang/OTP team label Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stalled waiting for input by the Erlang/OTP team team:PO Assigned to OTP team PO team:PS Assigned to OTP team PS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet