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.34% +0.17%
==========================================
Files 393 398 +5
Lines 98107 99164 +1057
==========================================
+ Hits 79629 80661 +1032
- Misses 18478 18503 +25
🚀 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 |
811dfd9 to
2aee690
Compare
|
Two of the failed CI tests are related. Could you please have a look |
2aee690 to
e2061c3
Compare
| log_j1939, | ||
| ) | ||
| from scapy.contrib.automotive.j1939.j1939_scanner import ( # noqa: F401 | ||
| _j1939_can_id, |
There was a problem hiding this comment.
Since all these definitions are used in multiple files, I recommend to use the leading "_"
888200f to
4a87d80
Compare
yes they sadly were. should be fixed now |
polybassa
left a comment
polybassa
left a comment
There was a problem hiding this comment.
Focus on simplicity and Scapy-likeness: prefer packet fields/answers()/sr1() over parallel decoders, opaque request bytes, and scanner-side reimplementation of correlation.
Inline notes cover NAME, Request/post_build, answers(), the DM scanner, and scanner result/sr1 handling.
This review was written with the help of AI (ChatGPT).
| return p + struct.pack("<I", target_pgn)[:3] | ||
| return p + pay | ||
|
|
||
| def answers(self, other): |
There was a problem hiding this comment.
Could we keep J1939.answers() limited to generic J1939 semantics?
Right now this method knows about several unrelated application interactions:
- DM14 -> DM15
- Diagnostic B -> Diagnostic A/B
- Diagnostic A -> Diagnostic A
- TP.CM RTS -> CTS/ABORT
- ECU-ID-specific BAM
- Command ACKs
- generic same-PGN fallback
For example, once DM15 is represented as a packet, the DM-specific relationship could live beside the DM packets instead:
class J1939_DM15(Packet):
PGN = J1939_PGN_DM15
def answers(self, other):
return isinstance(other, J1939_DM14)Likewise TP control-message correlation belongs naturally to the TP packet representation.
I would expect the generic layer to become closer to:
def answers(self, other):
if not isinstance(other, J1939):
return 0
if directed_request_does_not_match_addresses(self, other):
return 0
if isinstance(other, J1939Request):
return self.pgn == other.req_pgn
return 0The exact split can differ, but I would avoid turning J1939.answers() into a registry of every request/response pair that a future scanner might need.
There was a problem hiding this comment.
for DM13/14 : yep 100% .
For the TP things: I think that fast scanners actually depend on .answers() insights at this J1939 layer without the overhead of a ISO-TP transport socket (which might even be a kernel socket in some cases and we'd wait for streams)
for the DiagA / DiagB stuff : would it be better (from your / scapy's POV) to have a J1939DiagA() and J1939DiagB() classes to encapsulate pretty much only this answers() ? I don't think that class will have much more to it than that since diag is UDS 'tunneled' over tha PGN...
All of the current answers() logic is needed for the scanners to work so this stuff can't be removed, the current impl needs to be re-factored
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)
d4e4544 to
03cbd3e
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)
03cbd3e to
ffb02cd
Compare
| from scapy.supersocket import SuperSocket | ||
|
|
||
| try: | ||
| from scapy.contrib.cansocket_python_can import PythonCANSocket |
There was a problem hiding this comment.
Here should be a conditional import, on Linux nativeCanSocket should be preferred
| (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?
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.