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

Refactor offline sniff code #2559

Closed
wants to merge 2 commits into from

Conversation

roykuper13
Copy link
Contributor

@roykuper13 roykuper13 commented Mar 30, 2020

Few fixes:

  1. _write_to_pcap:
  • should use packets_list, but instead uses the "global"
    offline variable. So changed it to packets_list.
  • Returns filename twice. Changed it so it returns just once.
  1. in the isinstance checks part:
  • _write_to_pcap doesn't return offline. it returns the temp pcap file.
  • We do not check isinstance(offline, PacketList), which works as well,
    and should be exposed as an option for the user. Now it is.
  1. In case offline is a handle/path to a pcap file, the usage of the variable
    offline is misleading. Now it is more clearified.

  2. Update the comment of offline - this attribute
    can recieve also Packet, A list of Packets and a PacketList (not only pcap files).
    This should be mentioned as this feature is very usefull for these cases too (for example,
    filter packets that were sniffed using a raw socket).

In addition, I've noticed that when invalid bpf filter is passed to filter, wrong/misleading exception message is being raised (Not a supported capture file), so i've added the addition of (or invalid filter), so the user won't get confused.

@roykuper13 roykuper13 force-pushed the refactor-offline-sniff branch 2 times, most recently from cb6a6df to 31abcad Compare March 30, 2020 17:17
@codecov
Copy link

codecov bot commented Mar 30, 2020

Codecov Report

Merging #2559 into master will increase coverage by 0.71%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #2559      +/-   ##
==========================================
+ Coverage   88.28%   89.00%   +0.71%     
==========================================
  Files         243      246       +3     
  Lines       51006    54457    +3451     
==========================================
+ Hits        45032    48470    +3438     
- Misses       5974     5987      +13     
Impacted Files Coverage Δ
scapy/utils.py 85.75% <0.00%> (+5.87%) ⬆️
scapy/sendrecv.py 85.96% <100.00%> (-0.03%) ⬇️
scapy/compat.py 74.69% <0.00%> (-3.51%) ⬇️
scapy/main.py 72.55% <0.00%> (-0.66%) ⬇️
scapy/layers/tftp.py 89.39% <0.00%> (-0.29%) ⬇️
scapy/layers/smb.py 100.00% <0.00%> (ø)
scapy/contrib/bfd.py 91.66% <0.00%> (ø)
scapy/contrib/pfcp.py 98.66% <0.00%> (ø)
scapy/contrib/erspan.py 97.14% <0.00%> (ø)
scapy/layers/netbios.py 92.18% <0.00%> (+0.12%) ⬆️
... and 21 more

p-l-
p-l- previously approved these changes Mar 30, 2020
@p-l- p-l- requested review from guedou and gpotter2 March 30, 2020 19:23
@roykuper13 roykuper13 force-pushed the refactor-offline-sniff branch 3 times, most recently from 047e1f1 to a6c2707 Compare March 30, 2020 22:54
@roykuper13
Copy link
Contributor Author

About the pipeline failures -
Travis CI - #2558 (comment) pretty sure the same happened here

Codacy - I can understand the problem, but the problem was before as well (PcapReader never set the magic attribute..), so i'm not sure what should i do about that.

Scapy unit tests 3.7 - It failed in ARPingResult, and it seems that the problem isn't related to me, as I don't see any filter in this test. It is really hard to debug the failure in this one..
Moreover, when I run the test's lines locally from by branch

In [8]: ar = ARPingResult([(None, Ether(src='70:ee:50:50:ee:70')/ARP(psrc='192.168.0.1'))])

In [9]: with ContextManagerCaptureOutput() as cmco:
    ar.show()
    result_ar = cmco.get_output()
   ...:     

In [10]: result_ar.startswith("  70:ee:50:50:ee:70 Netatmo 192.168.0.1")
Out[10]: True

So.. i'm not sure what's going on.

@gpotter2
Copy link
Member

Unrelated random failure.

scapy/sendrecv.py Outdated Show resolved Hide resolved
scapy/sendrecv.py Outdated Show resolved Hide resolved
Few fixes:

1. _write_to_pcap:
- should use `packets_list`, but instead uses the "global"
offline variable. So changed it to `packets_list`.
- Returns `filename` twice. Changed it so it returns just once.

2. in the  `isinstance` checks part:
- _write_to_pcap doesn't return `offline`. it returns the temp pcap file.
- We do not check `isinstance(offline, PacketList)`, which works as well,
and should be exposed as an option for the user. Now it is.

3. In case `offline` is a handle/path to a pcap file, the usage of the variable
`offline` is misleading. Now it is more clearified.

4. Update the comment of `offline` - this attribute
can recieve also Packet, A list of Packets and a PacketList (not only pcap files).
This should be mentioned as this feature is very usefull for these cases too (for example,
filter packets that were sniffed using a raw socket).
if isinstance(offline, Packet) or \
isinstance(offline, PacketList) or \
(isinstance(offline, list) and
all(isinstance(elt, Packet) for elt in offline)):
Copy link
Member

Choose a reason for hiding this comment

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

If it's a list of something else than packets what happens? If the answer is "it crashes" then this line is unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @gpotter2

It doesn't crash, though the result can be considered "weird" - (The counter of Other will be equal to the list's length). I guess the right thing to do is to raise a TypeError or something.. I just copied the same type checks from the previous version to my changes.

Comment on lines +877 to +880
if isinstance(offline, Packet) or \
isinstance(offline, PacketList) or \
(isinstance(offline, list) and
all(isinstance(elt, Packet) for elt in offline)):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if isinstance(offline, Packet) or \
isinstance(offline, PacketList) or \
(isinstance(offline, list) and
all(isinstance(elt, Packet) for elt in offline)):
if isinstance(offline, (Packet, PacketList)) or \
(isinstance(offline, list) and
all(isinstance(elt, Packet) for elt in offline)):

@gpotter2
Copy link
Member

Made obsolete by #3055

@gpotter2 gpotter2 closed this Feb 11, 2021
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.

3 participants