L2 layer typing #2863

haggaie · 2020-10-11T14:05:08Z

Checklist:

If you are new to Scapy: I have checked CONTRIBUTING.md (esp. section submitting-pull-requests)
I squashed commits belonging together
I added unit tests or explained why they are not relevant
I executed the regression tests for Python2 and Python3 (using tox or, cd test && ./run_tests_py2, cd test && ./run_tests_py3)
If the PR is still not finished, please create a Draft Pull Request

I tried adding type hints as suggested in #2158 for the l2 layer this time.

I didn't squash everything together as I thought it would be easier to review this way. Let me know if you prefer otherwise.

To make mypy happy, I had to make a few changes to the config, packet, and field modules:

Avoiding adding attributes to Conf and NetCache after they were declared.
Modifying some of the type hints in Packet and Fields.
Using super() in some places instead of accessing unbound methods of the superclass directly.

codecov · 2020-10-11T14:24:41Z

Codecov Report

Merging #2863 (2660c8c) into master (711aa9f) will decrease coverage by 2.59%.
The diff coverage is 86.04%.

@@            Coverage Diff             @@
##           master    #2863      +/-   ##
==========================================
- Coverage   87.94%   85.35%   -2.60%     
==========================================
  Files         255      249       -6     
  Lines       54630    53308    -1322     
==========================================
- Hits        48046    45499    -2547     
- Misses       6584     7809    +1225

Impacted Files	Coverage Δ
scapy/layers/l2.py	`76.17% <81.25%> (-9.44%)`	⬇️
scapy/compat.py	`94.92% <100.00%> (-0.70%)`	⬇️
scapy/config.py	`81.29% <100.00%> (-3.06%)`	⬇️
scapy/fields.py	`91.64% <100.00%> (-0.12%)`	⬇️
scapy/packet.py	`81.54% <100.00%> (-0.22%)`	⬇️
scapy/arch/linux.py	`1.23% <0.00%> (-84.49%)`	⬇️
scapy/contrib/cansocket_native.py	`14.08% <0.00%> (-71.84%)`	⬇️
scapy/layers/tftp.py	`40.25% <0.00%> (-56.59%)`	⬇️
scapy/contrib/automotive/obd/scanner.py	`39.41% <0.00%> (-26.81%)`	⬇️
scapy/contrib/automotive/enumerator.py	`51.72% <0.00%> (-26.01%)`	⬇️
... and 86 more

scapy/layers/l2.py

gpotter2

Cool PR, you didn't pick an easy one :p
You'll however have to wait for #2859 to be merged first and it'll trigger a few conflicts.

haggaie · 2020-10-11T15:11:01Z

Cool PR, you didn't pick an easy one :p

Thanks, I realized that in the middle, but then I was already invested :)

You'll however have to wait for #2859 to be merged first and it'll trigger a few conflicts.

Sure, I understand.

gpotter2

Hi, could you rebase against master? Thanks

scapy/layers/l2.py

haggaie · 2020-10-29T13:48:58Z

Hi, could you rebase against master? Thanks

No problem.

scapy/packet.py

scapy/layers/l2.py

gpotter2 · 2020-11-15T15:57:03Z

scapy/layers/l2.py

@@ -408,41 +453,49 @@ class ARP(Packet):
                                         (val is None or valid_net6(val)))
                  ))),
            ],
-            StrFixedLenField("pdst", None, length_from=lambda pkt: pkt.plen),


I think we're going to disable the mypy issues for this check (that requires casting from Any) on all files in layers/ (that are not Core scapy), because it's going to become such a mess. Will get back to this

gpotter2 · 2020-11-15T16:13:32Z

To be perfectly honnest, we just started typing layers (most of the work was done on the core previously), so we're still trying to find good compromises between mypy rules and readable code. I'm sorry for that because I'm going to end up asking for tweaks several times. If that bothers you, I can take over your PR.

Packet fields attributes (e.g. pkt.plen in the comment above) cannot be typed, as they are generated dynamically, but adding cast() everywhere is gonig to be a mess. I've tweaked some rules in #2957 Could you remove those cast() calls, rebase on top of that PR and ignore the mypy issues? That would be awesome, thanks a lot !

haggaie · 2020-11-15T16:21:35Z

To be perfectly honnest, we just started typing layers (most of the work was done on the core previously), so we're still trying to find good compromises between mypy rules and readable code. I'm sorry for that because I'm going to end up asking for tweaks several times. If that bothers you, I can take over your PR.

I've been fine with the requests so far. It allows me to learn a little about mypy and scapy.

Packet fields attributes (e.g. pkt.plen in the comment above) cannot be typed, but adding cast() everywhere is gonig to be a mess. I've tweaked some rules in #2957 Could you remove those cast() calls, rebase on top of that PR and ignore the mypy issues? That would be awesome, thanks a lot !

Sure, I'd be happy too, but I've already rebased on top of #2954. Should I take the AnyField patch as well?

gpotter2 · 2020-11-15T17:20:21Z

Should I take the AnyField patch as well?

Yes that would be nice. I'll try to have it merged ASAP

gpotter2

This is starting to look really good !

Could you please rebase against master? The two PRs have been merged
Ideally you could also squash all commits at this point in time. Thanks a lot !

scapy/config.py

scapy/layers/l2.py

scapy/packet.py

scapy/layers/l2.py

Sequences in mypy allow variance, so we can assign lists of Field subtypes to fields_desc. This is needed for the scapy.layers.l2.Loopback class, which attempts to assign fields_desc twice with different types. Without this change, we get the following mypy errors: scapy/layers/l2.py:620: error: Incompatible types in assignment (expression has type "List[IntEnumField]", base class "Packet" defined the type as "List[Union[Field[Any, Any], _FieldContainer]]") scapy/layers/l2.py:620: note: "List" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance scapy/layers/l2.py:620: note: Consider using "Sequence" instead, which is covariant

The current code stores a pair of the address and the current time, but this does not seem to match the cache's code where the time is stored in a different field.

routing_info is a StrLenField, and accepts a lambda (now that the fld argument was removed).

haggaie · 2020-11-19T09:01:10Z

Ideally you could also squash all commits at this point in time.

I squashed most of the patches, but I left a few that seemed independent. Let me know if you want to squash them too.

gpotter2

Apart from one last minor comment, it's good to be merged to me. Thanks a lot for your time

scapy/layers/l2.py

- Add typed attribute for Conf.neighbor in scapy.config. This allows mypy to acknowledge the existence of this attribute even though it is not initialized until the scapy.layers.l2 module is loaded. - Name the arp cache as a private global in layers.l2 To avoid mypy warnings about arp_cache not being a member of the NetCache class, store it in its own variable. - Do not accept None in ConditionalField's condition callback The callback is always called with a packet. - The MACField internal representation may be None Modify the types and the i2repr implementation accordingly. - Allow StrFixedLenField default argument to be None A None default is passed in scapy.layers.l2 for example. - Explicitly return None in Neighbor.resolve() - Use super() to access MACField.i2h When accessing the i2h method using an unbound method, mypy fails matching the types correctly. - Convert a few string literals to bytes literals in scapy.layers.l2 - ShortEnumField accepts Dict[str, int] as well as Dict[int, str] - Do not accept None in StrFixedLenField length_from callback The callback is almost always called with a packet, and most of its usage is with a lambda function that does not handle None. Add `type: ignore` comment to the randval method that does pass None to this callback (inside a try-except clause).

* Change type of Packet.fields_desc to a Sequence Sequences in mypy allow variance, so we can assign lists of Field subtypes to fields_desc. This is needed for the scapy.layers.l2.Loopback class, which attempts to assign fields_desc twice with different types. Without this change, we get the following mypy errors: scapy/layers/l2.py:620: error: Incompatible types in assignment (expression has type "List[IntEnumField]", base class "Packet" defined the type as "List[Union[Field[Any, Any], _FieldContainer]]") scapy/layers/l2.py:620: note: "List" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance scapy/layers/l2.py:620: note: Consider using "Sequence" instead, which is covariant * Store just the hw address in the ARP cache in arping The current code stores a pair of the address and the current time, but this does not seem to match the cache's code where the time is stored in a different field. * Fix GRErouting.routing_info length routing_info is a StrLenField, and accepts a lambda (now that the fld argument was removed). * Add type annotations to scapy.layers.l2 - Add typed attribute for Conf.neighbor in scapy.config. This allows mypy to acknowledge the existence of this attribute even though it is not initialized until the scapy.layers.l2 module is loaded. - Name the arp cache as a private global in layers.l2 To avoid mypy warnings about arp_cache not being a member of the NetCache class, store it in its own variable. - Do not accept None in ConditionalField's condition callback The callback is always called with a packet. - The MACField internal representation may be None Modify the types and the i2repr implementation accordingly. - Allow StrFixedLenField default argument to be None A None default is passed in scapy.layers.l2 for example. - Explicitly return None in Neighbor.resolve() - Use super() to access MACField.i2h When accessing the i2h method using an unbound method, mypy fails matching the types correctly. - Convert a few string literals to bytes literals in scapy.layers.l2 - ShortEnumField accepts Dict[str, int] as well as Dict[int, str] - Do not accept None in StrFixedLenField length_from callback The callback is almost always called with a packet, and most of its usage is with a lambda function that does not handle None. Add `type: ignore` comment to the randval method that does pass None to this callback (inside a try-except clause).

haggaie force-pushed the l2-layer-typing branch from d022fbe to c66bb94 Compare October 11, 2020 14:06

gpotter2 added the Hinty label Oct 11, 2020

gpotter2 mentioned this pull request Oct 11, 2020

[Hinty] Add type hints to Scapy #2158

Closed

gpotter2 reviewed Oct 11, 2020

View reviewed changes

scapy/layers/l2.py Outdated Show resolved Hide resolved

gpotter2 reviewed Oct 11, 2020

View reviewed changes

haggaie force-pushed the l2-layer-typing branch 2 times, most recently from 4201897 to 67236e5 Compare October 19, 2020 14:08

gpotter2 reviewed Oct 29, 2020

View reviewed changes

scapy/layers/l2.py Outdated Show resolved Hide resolved

haggaie force-pushed the l2-layer-typing branch from 67236e5 to 2d77355 Compare October 31, 2020 23:56

haggaie commented Nov 1, 2020

View reviewed changes

scapy/packet.py Outdated Show resolved Hide resolved

gpotter2 mentioned this pull request Nov 14, 2020

Introduce AnyField type #2954

Merged

haggaie force-pushed the l2-layer-typing branch from 2d77355 to de06147 Compare November 15, 2020 14:05

gpotter2 reviewed Nov 15, 2020

View reviewed changes

haggaie force-pushed the l2-layer-typing branch from de06147 to e5545ec Compare November 15, 2020 19:08

gpotter2 reviewed Nov 17, 2020

View reviewed changes

scapy/layers/l2.py Outdated Show resolved Hide resolved

haggaie added 3 commits November 19, 2020 10:51

Store just the hw address in the ARP cache in arping

4c57764

The current code stores a pair of the address and the current time, but this does not seem to match the cache's code where the time is stored in a different field.

Fix GRErouting.routing_info length

ed46f7e

routing_info is a StrLenField, and accepts a lambda (now that the fld argument was removed).

haggaie force-pushed the l2-layer-typing branch from e5545ec to f0e7a7e Compare November 19, 2020 08:59

gpotter2 previously approved these changes Nov 19, 2020

View reviewed changes

scapy/layers/l2.py Outdated Show resolved Hide resolved

haggaie dismissed gpotter2’s stale review via 2660c8c November 22, 2020 07:04

haggaie force-pushed the l2-layer-typing branch from f0e7a7e to 2660c8c Compare November 22, 2020 07:04

gpotter2 approved these changes Nov 26, 2020

View reviewed changes

gpotter2 merged commit a16d2bf into secdev:master Nov 26, 2020

Uh oh!

L2 layer typing #2863

L2 layer typing #2863

Uh oh!

Conversation

haggaie commented Oct 11, 2020

Uh oh!

codecov bot commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

gpotter2 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haggaie commented Oct 11, 2020

Uh oh!

gpotter2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

haggaie commented Oct 29, 2020

Uh oh!

Uh oh!

Uh oh!

gpotter2 Nov 15, 2020

Choose a reason for hiding this comment

Uh oh!

gpotter2 commented Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haggaie commented Nov 15, 2020

Uh oh!

gpotter2 commented Nov 15, 2020

Uh oh!

gpotter2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

haggaie commented Nov 19, 2020

Uh oh!

gpotter2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Oct 11, 2020 •

edited

Loading

gpotter2 left a comment •

edited

Loading

gpotter2 commented Nov 15, 2020 •

edited

Loading