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

Inconsistent byte order for parameters passed to ndpi_network_ptree_match #81

Closed
bcronje opened this issue Aug 25, 2015 · 5 comments
Closed
Assignees

Comments

@bcronje
Copy link
Contributor

bcronje commented Aug 25, 2015

As a follow up to issue #77 , currently there is inconsistent usage of the byte order of the in_addr *pin parameter of ndpi_network_ptree_match

Based on the existing implementation of ndpi_network_ptree_match it appears ndpi_network_ptree_match expects in_addr *pin to be in host byte order, as you convert to network byte order looking at your comment here:

pin->s_addr = ntohl(pin->s_addr); /* Make sure all in network byte order otherwise compares wont work */

I'm assuming ndpi_patricia_search_best expects the prefix to be in network byte order then. However the problem is currently it's a bit of a mess how ndpi_network_ptree_match is called:

ndpi_host_ptree_match - calls ndpi_network_ptree_match with pin in host byte order
tor_ptree_match - calls ndpi_network_ptree_match with pin in network byte order
ndpi_detection_process_packet - calls ndpi_network_ptree_match with pin in network byte order
ndpi_guess_undetected_protocol - calls ndpi_network_ptree_match with pin in host byte order

Also related:

ndpi_init_ptree_ipv4 fills the prefix in network byte order
ndpi_add_host_ip_subprotocol fills the prefix in host byte order

Shouldn't this all be normalized so that whenever a struct in_addr is used that it should already be in network byte order, as the documentation and convention of struct in_addr is documented as network byte order: http://man7.org/linux/man-pages/man7/ip.7.html ?

@bcronje
Copy link
Contributor Author

bcronje commented Aug 26, 2015

@lucaderi do you guys want to tackle this internally? Or you happy if I take a shot at it?

@lucaderi
Copy link
Member

Go ahead

@bcronje
Copy link
Contributor Author

bcronje commented Aug 26, 2015

Great, should send through something later today.

@bcronje
Copy link
Contributor Author

bcronje commented Aug 26, 2015

OK PR #84 send. You have already done a lot of the work in your previous commit, so this PR addressed the outstanding endianess issues I could pick up.

@lucaderi
Copy link
Member

Thank you: all works as expected.

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

No branches or pull requests

3 participants