Fix bug with cache of payload in packetlistfield

[gpotter2]

• fixes https://github.com/secdev/scapy/issues/4411

edit: now also handles https://github.com/secdev/scapy/pull/4414#issuecomment-2180426716

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.46%. Comparing base (7dcb5fe) to head (7c7471e).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #4414 +/- ## ========================================== + Coverage 81.43% 81.46% +0.03% ========================================== Files 353 353 Lines 84258 84265 +7 ========================================== + Hits 68615 68648 +33 + Misses 15643 15617 -26 ``` | [Files](https://app.codecov.io/gh/secdev/scapy/pull/4414?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=secdev) | Coverage Δ | | |---|---|---| | [scapy/packet.py](https://app.codecov.io/gh/secdev/scapy/pull/4414?src=pr&el=tree&filepath=scapy%2Fpacket.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=secdev#diff-c2NhcHkvcGFja2V0LnB5) | `84.43% <100.00%> (+0.01%)` | :arrow_up: | ... and [42 files with indirect coverage changes](https://app.codecov.io/gh/secdev/scapy/pull/4414/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=secdev)

[LRGH]

This patch indeed fixes the non-regression that I have reported, but does not fix everything that has been broken by _raw_packet_cache_field_value

Based on the comment in test/fields.uts, it seems that scapy is no more intended to be usable when one want to parse a packet, change some of its content, and then build it again.

[gpotter2]

it seems that scapy is no more intended to be usable

That's not the case. Feel free to report any other regression you might encounter.

[LRGH]

Here is another regression.

from scapy.packet import Packet, bind_layers
from scapy.fields import PacketField, StrField

class PayloadPacket(Packet):
    fields_desc = [
        StrField("b", ""),
        ]

class SubPacket(Packet):
    fields_desc = [
        ]

bind_layers(SubPacket, PayloadPacket)

class MyPacket(Packet):
    fields_desc = [
        PacketField("a", None, SubPacket),
        ]

s = b'test'
p = MyPacket(s)
p.show()

p['PayloadPacket'].b = b'new'
p.show()
assert p.build() != s

[guedou]

@LRGH I have been following your latest comments for a few days now, and your behavior is hostile and borderline harassing towards Scapy maintainers. This is not welcome. Please stop immediately.

We maintain Scapy in our spare time, like many other open source contributors, and we don't deserve this.

You now have the opportunity to provide fixes and contribute to the project. Please be kind and do so instead of complaining and producing useless frictions.

[LRGH]

Indeed, I should not have had this behaviour. Please excuse me. I was upset by the comment "this is also a terrible idea" but I should not have asked the question the way I did.

[LRGH]

I will try to be more constructive.

It seems that all of this started with commit 1e1388d5540affe862421fc807022e12294c1d26 where a non-robust caching mechanism was introduced to improve the efficiency of do_build_payload. This mechanism was not robust because it did not detect when mutable fields are changed. Then commit 766b99b6c9ed1c9122643ef5e7da51486dacc6af introduced a way to deal with mutable fields, which was not efficient because the copy function was called during dissection. This inefficiency during dissection is the reason for patch 4aaed1d0a423ad8e9da571d4c1b1d105b84823a8 which breaks my code in multiple places.

I have made an experiment, by patching packet.py to always set raw_packet_cache to None. Almost all the non-regression tests of scapy are OK, which is what I expected, because when it is set to None the only impact should be on the performances (the raw packet value has to always be recomputed when needed). But there are too many tests that fail.

One example is the test Test chunked with gzip in test/scapy/layers/http.uts where there is a weird interaction between auto compression and chunk that I don't understand enough to find why it is dependent on the value of raw_packet_cache.

[LRGH]

I have created https://github.com/secdev/scapy/issues/4437 detected by disabling raw_packet_cache. There will be more.

[guedou]

Thanks @LRGH !

[LRGH]

The new patch still breaks some of my code, but it is likely that the reason is that I made some invalid assumptions when extending the functionality of some scapy internals. I am investigating.

[gpotter2]

If this is fine to you @guedou , feel free to merge it.