Split #3054: Refactoring of UDS Scanner#3352
Conversation
a5c01bd to
a066a18
Compare
c9c9b95 to
46b1a07
Compare
|
@gpotter2 This is ready for review |
bd73038 to
0421836
Compare
gpotter2
left a comment
There was a problem hiding this comment.
What's the candump_uds_scanner.log.gz for ?
scapy/contrib/automotive/bmw/hsfz.py
Outdated
There was a problem hiding this comment.
Thanks for asking... this requires a little bit of background information. Automotive ECUs which support HSFZ or DoIP usually have multiple different "States" and usually at least two interfaces, CAN and Ethernet. On state is the normal firmware, another state is for example the bootloader. Since the bootloader has a much smaller code base, than the firmware, in most cases, Ethernet (DoIP or HSFZ) is not supported. This means, as soon as an ECU enters the bootloader state, the Ethernet interface goes down.
This has some drawbacks for an automated scan, I want to do with UDS_Scanner. During a scan, the scanner will find a bootloader state and by default will try to run all enumerators against this bootloader to check which services are supported. The scanner will send an ECU to bootloader state, an then try to execute the enumerator. The enumerator will execute this line: https://github.com/secdev/scapy/blob/0421836e53447400668119edce8ae8843d6c678f/scapy/contrib/automotive/scanner/enumerator.py#L174-L176
Since, in the meantime the ECU put's its interface down, an Exception during send (_sndrcv_snd) will come up. Here is a very general catch:
Lines 251 to 252 in 23ff728
This catch now hides the exception from my enumerator. So the enumerator has now way to figure out, if the interface of the ECU went down. The enumerator will receive a None, and will try the next packet. This means the enumerator will fire a huge number of requests against a dead interface which floods the output with exception log messages.
I didn't want to change the code in _sndrcv_snd, therefore I did the exception handling inside this custom sockets. In this way, the socket goes down and the enumerator will know, that the socket died.
There was a problem hiding this comment.
I think that you should add some comments to the code =)
There was a problem hiding this comment.
I want to ensure that no pending messages in the queues of these two TestSocket objects are left. This basically drains these two Sockets
This file is a logfile of a scan of a real ECU. In the unit tests, I feed this logfile to an This is on the one hand, a good unit test to ensure that this "on-the-fly" cloning of a real world ECU's communication behavior works. Secondly, I use this virtual ECU (the answeringmachine) to run the UDS_scanner against it. |
0421836 to
63964c5
Compare
Codecov Report
@@ Coverage Diff @@
## master #3352 +/- ##
==========================================
- Coverage 85.28% 85.03% -0.26%
==========================================
Files 277 278 +1
Lines 57192 57623 +431
==========================================
+ Hits 48774 48997 +223
- Misses 8418 8626 +208
|
abfdbbd to
37071f7
Compare
e583dc1 to
7d6723e
Compare
a8ae38e to
ab28263
Compare
ab28263 to
d590186
Compare
|
What is the use case of propagating exceptions? We should move this discussion to a proper issue =) |
e5e2937 to
251cdc5
Compare
|
@guedou I've added all your remarks. Can this get merged? |
|
There is a failed test on macOS. I restarted the Github Actions jobs. It looks good to me, but I wonder if it is possible to slim down the pcap files. |
|
Hi, I already squeezed them to a minimum in order to fullfil their purpose. Another possibility would be, to host the pcap files somewhere and to fetch them. But this would have other downsides.
--
Viele Grüße
Nils Weiß
…On November 6, 2021 1:35:25 PM GMT+01:00, Guillaume Valadon ***@***.***> wrote:
There is a failed test on macOS. I restarted the Github Actions jobs.
It looks good to me, but I wonder if it is possible to slim down the
pcap files.
--
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
#3352 (comment)
|
|
Thanks for your answer! |
|
Is this Mac os error related to this PR?
--
Viele Grüße
Nils Weiß
…On November 6, 2021 4:35:20 PM GMT+01:00, Guillaume Valadon ***@***.***> wrote:
Thanks for your answer!
--
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
#3352 (comment)
|
|
Did you rebase against master? BTW, could you squash your commits? |
|
I will do both
--
Viele Grüße
Nils Weiß
…On November 6, 2021 4:38:32 PM GMT+01:00, Guillaume Valadon ***@***.***> wrote:
Did you rebase against master? BTW, could you squash your commits?
--
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
#3352 (comment)
|
ab82736 to
76d7644
Compare
|
All Done |
22ff139 to
4b5e185
Compare
|
@guedou This PR is ready from my side. I think the failed tests are not related to my changes, since some machines fail randomly always in the beginning of a test execution. |
guedou
left a comment
There was a problem hiding this comment.
LGTM. Thanlks @polybassa for your changes!
@gpotter2 I will let you perform an ultimate check as you reviewed this PR too.
No description provided.