Fix bug with cache of payload in packetlistfield

[gpotter2]

• fixes Regression with commit 4aaed1d0a423ad8e9da571d4c1b1d105b84823a8 #4411

edit:
now also handles #4414 (comment)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            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	Coverage Δ
scapy/packet.py	84.43% <100.00%> (+0.01%)	⬆️

... and 42 files with indirect coverage changes

[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 1e1388d 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 766b99b 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 4aaed1d 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 #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.

[LRGH]

It took me some time to extract from my code another issue.

from scapy.asn1.asn1 import ASN1_Class_UNIVERSAL

class ASN1_Class_DATA(ASN1_Class_UNIVERSAL):
    DATA = 0x80

from scapy.asn1.asn1 import     ASN1_ENUMERATED
from scapy.asn1.ber import  BERcodec_ENUMERATED
from scapy.asn1fields import   ASN1F_ENUMERATED

class ASN1_DATA(ASN1_ENUMERATED):
    tag = ASN1_Class_DATA.DATA
class BERcodec_DATA(BERcodec_ENUMERATED):
    tag = ASN1_Class_DATA.DATA
class ASN1F_DATA(ASN1F_ENUMERATED):
    ASN1_tag = ASN1_Class_DATA.DATA

from scapy.asn1.asn1 import ASN1_Codecs
from scapy.asn1packet import ASN1_Packet
from scapy.asn1fields import ASN1F_CHOICE

class PacketC(ASN1_Packet):
    ASN1_codec = ASN1_Codecs.BER
    ASN1_root = ASN1F_DATA("data",0,{})

class PacketB(ASN1_Packet):
    ASN1_codec = ASN1_Codecs.BER
    ASN1_root = ASN1F_CHOICE("DATA", PacketC(), PacketC)

from scapy.packet import Packet
from scapy.fields import PacketListField

class PacketA(Packet):
    fields_desc = [
        PacketListField("parts", [], PacketB),
        ]

input = b'\x80\x01\x03'

q = PacketA(input)
q[PacketC].data.val = 4
q = PacketA(q.build())

print( q[PacketC].data.val )

The output I expect is 4, but I see 3 instead.
If I disable all the raw packet cache functionality, I get 4 as expected.

Note that this is not only some theoretical mix of various scapy classes. It is based on my implementation of the TETRA ISI protocol, where there is an Ethernet frame that contains a SIP message that contains a ROSE payload (hence the ASN.1) that contains a TETRA PDU (for which I had to extend the implementation of ASN.1 to allow payloads of class Packet). And I want to be able to change some inner field and rebuild an Ethernet frame (or at least the SIP message).

[LRGH]

Another piece of code.

from scapy.packet import Packet
from scapy.fields import StrFixedLenField

class P(Packet):
    fields_desc = [
        StrFixedLenField('a', None, length=1),
        StrFixedLenField('b', None, length=1),
        ]

p = P(b'0')
s0 = p.build()
p.a = b'1'
s1 = p.build()
print(s0, s1)

The output is b'0' b'1\x00' with current scapy where raw packet cache is activated.
The output is b'0\x00' b'1\x00' with my patched scapy where raw packet cache is deactivated.

I would expect either b'0\x00' b'1\x00' or b'0' b'1'.
I have submitted #4450 as an independent issue because it is more related to the semantics of StrFixedLenField than to raw packet cache management.