Revert part of 2629

[gpotter2]

Revert a small part of #2629

I really don't like this addition: it slows guess_payload_class for a marginal use case. bind_layers should NOT allow callables.
The proper way has always been to add a custom guess_payload_class function when the bindings don't fit a bind_layers call.

It also seems that SMB2 is currently completly broken, mostly because of how it uses the FlagsFields. This fixes it

[codecov]

Codecov Report

Merging #2715 into master will decrease coverage by 0.03%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #2715      +/-   ##
==========================================
- Coverage   88.26%   88.22%   -0.04%     
==========================================
  Files         251      251              
  Lines       53351    53361      +10     
==========================================
- Hits        47090    47079      -11     
- Misses       6261     6282      +21     

Impacted Files	Coverage Δ
scapy/layers/smb2.py	98.03% <90.00%> (-1.97%)	⬇️
scapy/fields.py	92.61% <100.00%> (+0.01%)	⬆️
scapy/packet.py	81.48% <100.00%> (-0.07%)	⬇️
scapy/pton_ntop.py	89.04% <0.00%> (-8.22%)	⬇️
scapy/layers/tls/session.py	83.91% <0.00%> (-1.05%)	⬇️
scapy/utils.py	80.55% <0.00%> (-0.41%)	⬇️
scapy/sendrecv.py	86.56% <0.00%> (-0.17%)	⬇️
scapy/arch/windows/__init__.py	70.44% <0.00%> (-0.16%)	⬇️
scapy/layers/inet.py	74.05% <0.00%> (-0.09%)	⬇️
scapy/contrib/cdp.py	92.71% <0.00%> (+0.60%)	⬆️

[gpotter2]

This wasn't used by the FlagsField

[p-l-]

I do not agree (so far) with you (but maybe I just need to be convinced).

I think it may be good to have a flexible bind_layers() that would avoid specific, less readable implementations of .guess_payload_class().

What I really like with the current implementation, is that it allows to write statements that everyone will understand at first glance (which is, or at least should be, something we should keep in mind when writing dissectors).

I do agree with you: such a change just for one situation is not worth it. But I think that this change might make it possible to rewrite several dissectors in current Scapy code so that they get more readable, and to write complex layer binds using readable and simple code.

[gpotter2]

I'm going to check the performance impact of such a change then make up my mind. guess_payload_class is already one of the place where scapy spends the more time, and I don't want that to become even worse.

I do agree that the use of lambdas in bind_layers looks nicer. But maybe it could wait for after we're done with 2.4.4 ?

[p-l-]

I'm going to check the performance impact of such a change then make up my mind. guess_payload_class is already one of the place where scapy spends the more time, and I don't want that to become even worse.

It is possible that a performance impact exists. On the other hand, Scapy's trade-off usually favors dissector's readability over performances (if you want to analyze a PCAP file and need high performances, you are probably not going to use Scapy anyway); so I guess it all depends on how bad the impact will be.

I do agree that the use of lambdas in bind_layers looks nicer. But maybe it could wait for after we're done with 2.4.4 ?

It could have waited, that is totally true. However now it is in the codebase, I don't think we should remove it, only to restore it after 2.4.4. It is an important change, but it does not break or anyhow affect things that work with 2.4.3 (that would have been a good reason to discuss the removal).

[gpotter2]

I've ran some tests, this has almost no performance impact. I've re-added the changes

[guedou]

LGTM. Why did you remove tests from smb2.uts?

[gpotter2]

Those tests don't test anything, and had funky spacing. I also had issue on a machine because of the ~SMB2 tag misplaced at the top

[guedou]

@gpotter2 thanks for your answer.

@p-l- your call!

[p-l-]

I think we could have the SMB2 tests kept, with in pkt replaced by in IP(raw(pkt)). What do you think @gpotter2?

[gpotter2]

@p-l- I actually have another major issue with this proposal: using a lambda in bind_layers breaks building.
If I take back the example used in SMB2:

bind_layers(
    SMB2_Header,
    SMB2_Negociate_Protocol_Response_Header,
    Command=0x0000,
    Flags=lambda f: (f >> 24) & 1 == 1
)

Flags won't be edited on building. This is messed up and not what we want for bind_layers, since it's supposed to be a both-way function. In this matter I think using a guess_payload_class and a bind_top_down call is much cleaner.

The test you pointed out actually shows that this is broken: my only fix here would be to add an extra bind_top_down call.

[p-l-]

@p-l- I actually have another major issue with this proposal: using a lambda in bind_layers breaks building.

OK that's a killer argument IMO.