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

Fix OSPF LSA list dissection #751

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@illordlo
Contributor

illordlo commented Aug 7, 2017

In the PacketListField constructor, inside the OSPF_LSUpd class, an empty list was passed as argument. This resulted into strange behaviour, leading to the LSA list being displayed and managed as a raw set of bytes.

Substituting the empty list with None let the PacketListField constructor create its own empty list, resulting with the dissection process working as expected.

@codecov-io

This comment has been minimized.

codecov-io commented Aug 7, 2017

Codecov Report

Merging #751 into master will decrease coverage by 0.74%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #751      +/-   ##
==========================================
- Coverage   80.29%   79.55%   -0.75%     
==========================================
  Files         138      137       -1     
  Lines       33939    33845      -94     
==========================================
- Hits        27251    26925     -326     
- Misses       6688     6920     +232
Impacted Files Coverage Δ
scapy/fields.py 89.45% <ø> (+0.09%) ⬆️
scapy/arch/bpf/core.py 25.64% <0%> (-56.42%) ⬇️
scapy/layers/dhcp6.py 62% <0%> (-22.9%) ⬇️
scapy/arch/__init__.py 72.72% <0%> (-14.55%) ⬇️
scapy/arch/unix.py 60.09% <0%> (-11.54%) ⬇️
scapy/data.py 81.17% <0%> (-2.06%) ⬇️
scapy/utils.py 76.96% <0%> (-1.71%) ⬇️
scapy/sendrecv.py 46.16% <0%> (-1.44%) ⬇️
scapy/layers/dhcp.py 84.3% <0%> (-1.35%) ⬇️
scapy/layers/dns.py 88.2% <0%> (-0.57%) ⬇️
... and 10 more
@illordlo

This comment has been minimized.

Contributor

illordlo commented Aug 7, 2017

BTW, this PR should also fix the entry "Support incomplete payload in ospf.py" in #399 .

I did some tests and they worked as expected, but please, double check what I am proposing because I am new to Scapy and this is my first PR on this project.

@p-l-

This comment has been minimized.

Member

p-l- commented Aug 11, 2017

Hi,

Thanks! Can you add a test case with something that would not work correctly (or even crash) with current code and works properly with your code? This will help us prevent regressions in the future.

@illordlo

This comment has been minimized.

Contributor

illordlo commented Aug 13, 2017

Hi!

I did it but I don't know how to test it. I just used another existing file as a template, testing single commands on the Scapy interactive interface, so it should work. The previous code was not able do dissect the OSPF_Router_LSA field, while with the fix proposed everything should be accessible and now seen as "Raw".

If you could confirm it this work or where it should be fixed, if needed, then I would really appreciate it.

Thanks in advance!

@guedou guedou referenced this pull request Sep 14, 2017

Merged

Improved PR#751 #811

@guedou

This comment has been minimized.

Member

guedou commented Sep 14, 2017

@illordlo thanks for the unit tests ! You were almost there =)

I modified you PR in #811 to:

  • move ospf.uts to scapy/contrib/ospf.uts and automatically trigger the tests
  • edit the file to use the corret format

@guedou guedou closed this Sep 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment