Skip to content

Fix for issue #1199#1276

Closed
polybassa wants to merge 1 commit intosecdev:masterfrom
polybassa:issue_1199_fix
Closed

Fix for issue #1199#1276
polybassa wants to merge 1 commit intosecdev:masterfrom
polybassa:issue_1199_fix

Conversation

@polybassa
Copy link
Copy Markdown
Contributor

No description provided.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 21, 2018

Codecov Report

Merging #1276 into master will decrease coverage by 5.53%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
- Coverage   84.79%   79.25%   -5.54%     
==========================================
  Files         160      159       -1     
  Lines       38322    37544     -778     
==========================================
- Hits        32495    29757    -2738     
- Misses       5827     7787    +1960
Impacted Files Coverage Δ
scapy/sendrecv.py 40.26% <100%> (-38%) ⬇️
scapy/layers/tls/automaton.py 15.78% <0%> (-64.92%) ⬇️
scapy/arch/pcapdnet.py 12.62% <0%> (-56.15%) ⬇️
scapy/as_resolvers.py 32.07% <0%> (-49.06%) ⬇️
scapy/layers/tls/automaton_srv.py 26.2% <0%> (-45.67%) ⬇️
scapy/layers/tls/automaton_cli.py 31.88% <0%> (-45.15%) ⬇️
scapy/supersocket.py 38.96% <0%> (-40.26%) ⬇️
scapy/modules/nmap.py 57.46% <0%> (-39.56%) ⬇️
scapy/arch/linux.py 36.5% <0%> (-37.37%) ⬇️
... and 41 more

@guedou
Copy link
Copy Markdown
Member

guedou commented Mar 21, 2018

Thanks for the PR. Could you provide unit tests?

For example:

>>> packet = IP(dst="8.8.8.8")/ICMP() ; r = sr(packet); packet.sent_time is not None
>>> packet = IP(dst="8.8.8.8")/ICMP() ; r = sr1(packet); packet.sent_time is not None

@karpierz karpierz mentioned this pull request Mar 22, 2018
@guedou
Copy link
Copy Markdown
Member

guedou commented Mar 22, 2018

It was only examples. Please remove the ';' and add assert before the tests for None. Thanks.

@polybassa polybassa force-pushed the issue_1199_fix branch 2 times, most recently from c4e779d to 07f4f57 Compare March 22, 2018 08:34
@polybassa
Copy link
Copy Markdown
Contributor Author

Sorry

@polybassa polybassa force-pushed the issue_1199_fix branch 3 times, most recently from 2c4c74a to 9e35550 Compare March 22, 2018 14:59
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Your first proposal list was correct, this one isn’t. A packet may be a generator, a list...

@karpierz
Copy link
Copy Markdown
Contributor

Why not?:
tobesent = list(pkt) if hasattr(pkt, "__iter__") else pkt

@gpotter2
Copy link
Copy Markdown
Member

I think it's safe to call simply list(pkt):

  • if pkt is a list, list() returns the same object
  • if pkt is a packet, or a generator, iter is implicitly called

@polybassa
Copy link
Copy Markdown
Contributor Author

Now I got a problem. If I do the following:
tobesent = [pkt]
this issue is fixed, but some other unit test will fail.
if I do
tobesent = list(pkt)
this issue is still present, but other unit test will not fail.

@gpotter2
Copy link
Copy Markdown
Member

gpotter2 commented Mar 22, 2018

This means that the fix is incorrect. this line is more complex than it looks, knowing that a packet can generate others depending on its parameters

@karpierz
Copy link
Copy Markdown
Contributor

karpierz commented Mar 22, 2018

I think it's safe to call simply list(pkt):
if pkt is a list, list() returns the same object

It is not quite true.
list(list_object) returns a new list object (copy of list_object). Try:
a = [1,2,3,4]
b = list(a)
a is b

@polybassa
Copy link
Copy Markdown
Contributor Author

polybassa commented Mar 22, 2018

ok, the problem is probably somewhere inside packet.__iter__

@polybassa
Copy link
Copy Markdown
Contributor Author

unfortunately, I don't know enough internals of scapy, to fix this issue

@karpierz
Copy link
Copy Markdown
Contributor

I will try to help you.
UPDATE: Solved. See at #1282

@polybassa polybassa deleted the issue_1199_fix branch May 7, 2018 07:17
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.

5 participants