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

DoIP improvements all-around #510

Merged
merged 21 commits into from
Apr 3, 2024
Merged

DoIP improvements all-around #510

merged 21 commits into from
Apr 3, 2024

Conversation

ferdinandjarisch
Copy link
Contributor

This PR contains multiple fixes that:

  • Improve logging output of intermediate results and summaries
  • Streamline how connection errors are treating in RoutingActivationType/SourceAddress enumeration
  • Avoid stalling enumerations due to dangling socket connections when enumerating SourceAddresses

@ferdinandjarisch ferdinandjarisch mentioned this pull request Feb 28, 2024
2 tasks
@rumpelsepp
Copy link
Member

What' the rational of b8c3ce0? This code seems unfortunate to me. Why is writing random data to a connection needed to close it?

@ferdinandjarisch
Copy link
Contributor Author

What' the rational of b8c3ce0? This code seems unfortunate to me. Why is writing random data to a connection needed to close it?

As mentioned in the log message and according to what you wrote yourself, the rationale is to "to help our kernel realize the connection is closed to avoid TIME_WAIT".

Apparently this is due to implementation details of asyncio's open_connection, having a stream reader/writer on both ends that do not directly communicate, iirc.
Writing EOF alone is not sufficient.

In any case, without this change the DoIP discoverer occupies up to 13.000 Ports while enumerating, as they are not properly closed.

src/gallia/commands/discover/doip.py Fixed Show resolved Hide resolved
@ferdinandjarisch
Copy link
Contributor Author

With recent changes this PR now fully surpasses #506

Additional content includes:

  • Fixes treatment of DiagnosticMessageACKs to match requirements from Standard
  • Exposes DoIP protocol version to target string
  • Auto-detects protocol_version during discovery
  • Queries additional content from DoIP entity via DoIPEntityStatusRequests

@ferdinandjarisch ferdinandjarisch changed the title doip-discover fixes DoIP improvements all-around Mar 7, 2024
@ferdinandjarisch
Copy link
Contributor Author

And even more feats:

  • --tcp-connect-delay flag to slow down enumerations, which can be useful for not-so-powerful DoIP entities
  • Dynamic scanning of RoutingActivationType and SourceAddress, depending on what exactly the DoIP entity prefers

Apart from that some minor fixups for previous commits; so once done this needs to be rebased

@rumpelsepp
Copy link
Member

For the record, discussed via matrix. ACK, but these points are open:

  1. Use asyncio for special UDP socket; supported since Python 3.11: https://docs.python.org/3/library/asyncio-eventloop.html#working-with-socket-objects-directly
socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP)
sock.setblocking(False)
sock.bind(("0.0.0.0", 0))
loop = asyncio.get_running_loop()
data, addr = await loop.sock_recvfrom(sock, …)
await loop.sock_sendto(sock, …)
  1. Due to a future refactor, avoid defining new class variables.
  2. Make the socket option SO_LINGER optional. This is a special case for the discovery scanner and might cause unexpected repercussions for other scanners.

src/gallia/commands/discover/doip.py Dismissed Show dismissed Hide dismissed
It is up to the DoIP entity to decide whether it first checks for
correct SourceAddress or RoutingActivationType (while the standard
actually puts the check for SourceAddress first...).
This commit minimizes the search space by determining which check is
performed first, and accordingly adapting the following scans.
By doing so it also removes code duplication.
@ferdinandjarisch
Copy link
Contributor Author

All comments should be addressed.

Maybe let it rest for a day to let me double check tomorrow, otherwise it should be good to merge.

@ferdinandjarisch
Copy link
Contributor Author

And for the record: this MR fully surpasses and thus closes #506

@ferdinandjarisch ferdinandjarisch merged commit 2286f60 into master Apr 3, 2024
11 checks passed
@ferdinandjarisch ferdinandjarisch deleted the doip-discover-fixes branch April 3, 2024 20:51
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

Successfully merging this pull request may close these issues.

2 participants