Skip to content

Sendrecv overhaul: async sniffing & major cleanup #1999

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

Merged
merged 13 commits into from
Jun 14, 2019

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Apr 26, 2019

This PR:

  • ✔️ REQUIRES Fix FlagsField & dissection improvements #2035
  • contains an alternative fix for packet.time_sent can't be set because of object copy during list comprehension #1199, that binds the sent_time in __iter__ rather than requiring extra memory space to store the timestamps. This brings a small performance boost, see below.
  • Implements AsyncSniff, removes tons of duplications, and allow us to use Sessions everywhere
    • sniff uses AsyncSniff
    • sndrcv uses AsyncSniff
    • sndrcvflood uses sndrcv.
  • use classes to handle sniffing and sndrcv: they allow greater flexibility when sharing properties than the previous method.
  • drops Event usage: not required anymore if we use a class.
  • removes process from sndrcv: no one uses it, and it's extremly messy. (My bad). We could reimplement it much easily now that we use AsyncSniffer within sndrcv
  • fixes heavy memory leaks in scapy/automaton.py
  • Add docs about AsyncSniffer
  • Change of behavior: packets that are None are ignored on all platforms (previously only Windows/BPF), instead of closing the socket (and using the very unclear TimeoutElapsed). EOFError will now be the default exception to close a socket (this behavior is only used in *PcapReader)
  • fixed BSD bug: conf.use_bpf would always enable itself even if conf.use_pcap was True. (Missing parenthesis)
  • migrates scapy.arch.bpf.supersocket sockets to use the recv_raw mechanic

It's a bit harder to implement than it looks, because it must be able to get out of a frozen select(), on sockets that don't get any more packets.

Perfs:

Using the benchmarking of #1259

Note: the following stats should only be read relatively speaking. The test is performed using a short packet (Ether()/IP()) on loopback. It isn't intended to match a real usage by any mean.

secdev/master gpotter2/perfsndrcv
Python 2.7 ~ 5700/s ~ 5900/s
Python 3.7 ~ 6800/s ~ 7300/s

Notes:

async_select_unrequired=False means that the select function is blocking, therefore a control socket (fake) should be passed. It should be set to True otherwise (Windows/OSX/Pcapdnet)... We could potentially rename it is_blocking_socket=False if it's clearer (but I only thought of that later 😄)

Hopes for the future:

Thanks to the use of *sniff within the sndrcv suite, we have less code to maintain + it will allow us to enable:

  • Sessions in sndrcv*: match a chunked HTTP response to its answer ?
  • multi-thread dissection (I have great hopes for this. I plan to use multiprocessing.dummy and try to paralellize packet dissections in pools). Edit: to anyone still reading this, note AsyncSniffer does NOT improve performances (or barely). Splitting the work load in multiple threads is useless. We could try to split it across processes, but that makes it much harder to implement. Eventually, the best option to optimize your code is to disable dissection of packets you don't use. We should make an util to make that easier someday

Related:

fixes #1523
fixes #1505
fixes #1937

@gpotter2 gpotter2 changed the title Improve memory usage during sndrcv. Improve memory usage during sndrcv Apr 26, 2019
@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #1999 into master will increase coverage by 0.27%.
The diff coverage is 86.05%.

@@            Coverage Diff             @@
##           master    #1999      +/-   ##
==========================================
+ Coverage   86.86%   87.14%   +0.27%     
==========================================
  Files         197      197              
  Lines       44505    44512       +7     
==========================================
+ Hits        38661    38791     +130     
+ Misses       5844     5721     -123
Impacted Files Coverage Δ
scapy/contrib/isotp.py 88.32% <100%> (+0.01%) ⬆️
scapy/contrib/cansocket_native.py 80.55% <100%> (+0.27%) ⬆️
scapy/packet.py 79.67% <100%> (+0.18%) ⬆️
scapy/layers/l2.py 84.23% <100%> (+0.03%) ⬆️
scapy/arch/common.py 89.65% <100%> (-0.57%) ⬇️
scapy/contrib/cansocket_python_can.py 91.93% <100%> (+0.13%) ⬆️
scapy/arch/windows/native.py 79.24% <100%> (+0.39%) ⬆️
scapy/automaton.py 85.44% <100%> (+3.34%) ⬆️
scapy/scapypipes.py 86.61% <100%> (-0.11%) ⬇️
scapy/arch/pcapdnet.py 69.65% <100%> (-1%) ⬇️
... and 22 more

@gpotter2 gpotter2 changed the title Improve memory usage during sndrcv [WIP] Improve memory usage during sndrcv Apr 26, 2019
@gpotter2 gpotter2 force-pushed the perfsndrcv branch 2 times, most recently from aa62755 to 6a36830 Compare May 18, 2019 19:09
@gpotter2 gpotter2 changed the title [WIP] Improve memory usage during sndrcv Improve memory usage during sndrcv May 18, 2019
@gpotter2 gpotter2 changed the title Improve memory usage during sndrcv [WIP] Improve memory usage during sndrcv May 18, 2019
@gpotter2 gpotter2 changed the title [WIP] Improve memory usage during sndrcv Sendrecv cleanup & Memory improvements ! May 18, 2019
@gpotter2 gpotter2 changed the title Sendrecv cleanup & Memory improvements ! Sendrecv cleanup & Memory improvements & FlagsFields bug May 18, 2019
@gpotter2 gpotter2 force-pushed the perfsndrcv branch 2 times, most recently from a4dba5b to eab7908 Compare May 18, 2019 21:43
@gpotter2 gpotter2 changed the title Sendrecv cleanup & Memory improvements & FlagsFields bug [WIP] Sendrecv cleanup & Memory improvements & FlagsFields bug May 18, 2019
@gpotter2 gpotter2 changed the title [WIP] Sendrecv cleanup & Memory improvements & FlagsFields bug [WIP] Sendrecv cleanup May 19, 2019
@gpotter2 gpotter2 force-pushed the perfsndrcv branch 4 times, most recently from 6bb9e76 to 5ee6640 Compare May 19, 2019 16:27
@gpotter2 gpotter2 changed the title [WIP] Sendrecv cleanup [WIP] Sendrecv overhaul May 25, 2019
@gpotter2
Copy link
Member Author

gpotter2 commented Jun 2, 2019

@polybassa I finally managed to fix BSD*. The last commit fails because of isotp.uts tests (see the Travis build: https://travis-ci.com/secdev/scapy/builds/114015453) :/

The PR nearly is in its final state, the only thing left is to fix that. It works all right if I disable those.
I'll try to figure it out, but you're much more familiar with this part than I am. I would be very grateful if you could have a (another?) look.

Also, note that this PR will probably allow to rewrite CANReceiverThread and ISOTPSniffer, in a prettier way.

@polybassa
Copy link
Contributor

@gpotter2
The tests that are failing, are tests with a mocked socket. This means, the problem is not related to vcan or to CANSockets. Either the implementation of the MockSocket has a bug or the implementation of ISOTPSoftSocket is the problem. Maybe @epozzobon can help us here.

@gpotter2 gpotter2 mentioned this pull request Jun 3, 2019
27 tasks
@gpotter2
Copy link
Member Author

gpotter2 commented Jun 3, 2019

@polybassa You were very right ! Thanks a lot, I had missed the CANMockSocket 😄

I've updated it using the new mechanic in be21a50, and it appears to have solved the issue !

@gpotter2 gpotter2 changed the title [WIP] Sendrecv overhaul: async sniffing & major cleanup Sendrecv overhaul: async sniffing & major cleanup Jun 3, 2019
@gpotter2
Copy link
Member Author

gpotter2 commented Jun 8, 2019

@guedou @p-l- This PR is ready to be reviewed.
It's quite big, and it did expand a bit more than I expected. I however managed to split the changes in different commits, so following them individually should do.

It's still a pretty cool feature

Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to take more time to review the changes in sendrecv.py but I like the idea of refactoring the code!

Copy link
Member

@p-l- p-l- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is brilliant. Thanks (and congrats) @gpotter2!

@p-l- p-l- merged commit fcab768 into secdev:master Jun 14, 2019
@porceCodes
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Performs some code clean-up enhancement major Major changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide stop for sniff even when no packets received
5 participants