automotive, j1939: scanning for CAs - #5164
BenGardiner wants to merge 5 commits into
Conversation
|
thanks @polybassa for the review. I can do almost all of that right now. There's a couple things that are either answering your questions or require me to ask you questions first. |
fbcb5a9 to
4757249
Compare
|
I noticed that in the rebase of the scanner code to your replacement soft socket the scanners were no longer cleanly relying on sr() / sr1() via answers() logic. I'll work on bringing that back, fixing the things above I didn't have questions about and then refactoring the scanners to use the answers() logic... |
97acab8 to
f664c8a
Compare
|
Hi @polybassa while I think this is ready for your next review, it might not be merged in this form. There are 'Feature' commits and then 'fixes' on them. e.g. FFfffFffFfffffFfff. To merge you would probably want the fixes squashed into the features. You may even want one squashed commit -- in which case you might consider merging the first commit separately since it is implementing missing sr1() functionality in the current J1939SoftSocket on master. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5164 +/- ##
==========================================
+ Coverage 81.16% 81.30% +0.13%
==========================================
Files 393 398 +5
Lines 98107 99350 +1243
==========================================
+ Hits 79629 80776 +1147
- Misses 18478 18574 +96
🚀 New features to boost your workflow:
|
3b5967c to
4d7cb29
Compare
|
I'm going to look closer at https://github.com/secdev/scapy/actions/runs/35222531093/job/105206012569?pr=5164 -- that seems like there could be something wrong with the soft socket... |
yep that was a race in close() of the soft socket -- just like we had in isotp soft socket. I have a fix |
35d5188 to
625c334
Compare
|
sorry the checks may be skipped now due to a rate limit... I think I resolved them but pretty hard to tell locally without waiting for the github runners |
625c334 to
58adda4
Compare
|
no sorry. skipped due to a typo. |
58adda4 to
811dfd9
Compare
|
ok... ok well that was a journey... over to you then @polybassa |
4a87d80 to
766ca0c
Compare
|
Please have a look on the failed ci tests |
7109f14 to
d4e4544
Compare
yup sorry about that -- the import CANSocket pattern is pretty fragile for my agents it seems. |
AI-Assisted: Yes Kimi 2.7 / GPT 5.4 codex / Copilot
…allback (heuristic) Implement answers(), clone_with(), and copy() on J1939 to support sr1() for directed and broadcast requests with session tracking and fallback heuristics. Add identifier property and setter to J1939_CAN to satisfy the CAN interface for python-can backends, and add a defensive fallback in _can_send() converting to CAN on AttributeError. AI-Assisted: yes (Gemini 3.8 Flash)
03cbd3e to
ffb02cd
Compare
| (preserving the underlying CAN socket). | ||
| """ | ||
| if reconnect_handler is not None: | ||
| probe = reconnect_handler() |
There was a problem hiding this comment.
This code could be unified with the code above
| # byte 3: total packets = 2 | ||
| # byte 4: max packets per CTS = 0xFF (no limit) | ||
| # bytes 5-7: PGN being transferred (probe PGN = 0x0000FF) | ||
| rts_payload = struct.pack( |
There was a problem hiding this comment.
Why not using a scapy packet here?
| def clone_with(self, payload=None, **kargs): | ||
| # type: (Optional[Any], **Any) -> J1939 | ||
| pkt = super(J1939, self).clone_with(payload=payload, **kargs) | ||
| pkt.priority = kargs.get('priority', self.priority) | ||
| pkt.pgn = kargs.get('pgn', self.pgn) | ||
| pkt.src = kargs.get('src', self.src) | ||
| pkt.dst = kargs.get('dst', self.dst) | ||
| return pkt | ||
|
|
||
| def copy(self): | ||
| # type: () -> J1939 | ||
| clone = super(J1939, self).copy() | ||
| clone.priority = self.priority | ||
| clone.pgn = self.pgn | ||
| clone.src = self.src | ||
| clone.dst = self.dst | ||
| return clone |
There was a problem hiding this comment.
This is a bit weird, what's it for?
There was a problem hiding this comment.
yes. yes it is weird . This exists because the transport protocol sessions are in information in the CAN ID; and the CAN ID isn't in the payload of the message.
In more detail (some of this is in the interim comments above, but this is hopefully much clearer)
We had a problem iterating over a packet ([x for x in pkt] or for p in to_send):
-
internally invokes Packet.iter(), which yields packets created via self.clone_with(payload=payl, **done2).
-
When using sr1(J1939(data, pgn=0xFECA, dst=0x10)), the packet pipeline iterates and copies the sent packet.
-
When an incoming frame arrived, J1939.answers(other) was evaluated against other. Without these overrides, other.pgn, other.dst, and other.src had reverted to 0 and 0xFF, causing answers() to fail to
correlate the response with the request.
Overriding clone_with() and copy() on J1939 ensures that priority, pgn, src, and dst are preserved during copies, cloning, and sr1() iteration
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def j1939_get_sock( |
There was a problem hiding this comment.
j1939_scanner.py currently contains quite a lot of socket-framework code (_j1939_get_cansocket_cls, _open_sa_filtered_sock, j1939_resolve_probe_sock, j1939_resolve_broadcast_sock, j1939_get_sock). This makes the individual scan methods much harder to follow and couples J1939 scanning to concrete CAN socket implementations.
Could we reduce this to one socket boundary and let the scan implementations operate on a J1939Socket? For example:
@contextmanager
def _j1939_socket(sock, reconnect_handler=None, **kwargs):
if reconnect_handler is None:
with J1939Socket(sock, **kwargs) as s:
yield s
return
raw = reconnect_handler()
try:
with J1939Socket(raw, **kwargs) as s:
yield s
finally:
raw.close()Then a scan should ideally be little more than construct J1939Request → sock.sr1(...).
If per-target CAN filtering is required for performance, I'd prefer exposing that via a socket factory / CANSocket configuration instead of having the J1939 scanner detect PythonCANSocket/NativeCANSocket, recover the channel, and reconstruct the backend itself.
| PythonCANSocket = None # type: ignore[assignment, misc] | ||
|
|
||
|
|
||
| def _j1939_get_cansocket_cls(): |
There was a problem hiding this comment.
Importing and branching on concrete CAN backends (PythonCANSocket / NativeCANSocket) inside the J1939 scanner feels like a layer violation. A J1939 scan shouldn't need to know which CANSocket alias loaded, how to recover .channel, or how to reconstruct an equivalent filtered socket.
Prefer either using the passed socket as-is, or an explicitly supplied reconnect_handler / socket factory—without reverse-engineering the backend. That also avoids the kind of import/CI fragility already noted in this PR.
| # --- Technique: unicast DM PGN probe | ||
|
|
||
|
|
||
| def j1939_scan_dm_pgn( |
There was a problem hiding this comment.
This remains one of the clearer non-Scapy-like parts: the docstring notes it could use .sr1(), but the implementation manually builds the request, flushes, registers a sniff callback, parses responses, early-stops, and paces.
I'd prefer the Scapy-like baseline first:
req = J1939Request(req_pgn=pgn, dst=target_da, src=src_addr)
resp = sock.sr1(req, timeout=sniff_time, verbose=False)with answers() expressive enough for direct PGN response / NACK / TP announcement as needed. Optimize the hot path only after measuring that normal sr1() is inadequate—this would remove a surprising amount of custom scanner code.
| ps = self.pgn & 0xFF | ||
| return j1939_to_can_id(self.priority, 0, dp, pf, ps, self.src) | ||
|
|
||
| def answers(self, other): |
There was a problem hiding this comment.
Fast scanners can benefit from correlation without a full transport socket, but I wouldn't put every scanner relationship into generic J1939.answers(). Prefer splitting by packet type, e.g. J1939_TP_CM_CTS.answers() matching RTS by PGN, and likewise for DM/application packets.
J1939.answers() should stay limited to envelope concerns (src/dst where applicable, request-PGN matching, maybe generic PGN equivalence)—not a database of scanner-specific conversations. That still gives sr1() cheap answers() dispatch without needing a complete TP session.
|
Hi Ben, here is some final review. I think we are almost ready to merge. Thanks for your patience |
e324023 to
cea01b5
Compare
Adds a scanner to identify Controller Applications in a J1939 network, various scanning techniques are provided including both broadcast and unicast. AI-Assisted: yes (Gemini 3.8 Flash)
AI-Assisted: yes (Gemini 3.8 Flash)
…939-81) AI-Assisted: yes (Gemini 3.8 Flash)
cea01b5 to
b99ba54
Compare
Description
This adds j1939 scanning for Controller Applications on-top-of the soft socket support.
The changes aim to introduce only those J1939 value enumeration definitions which can be sourced from freely available locations on the internet. As such, there is not a complete list of the values.
I don't intend any impacts on other parts of the libraries.
fixes missing sr1() functionality in J1939 soft socket on master
LLM coding tools were used in the development of this PR: copilot and gemini, various models.