Oddities with FieldListField

[LRGH]

I have been looking at the internals of FieldListField and would like to suggest some simple changes.

• Looking at FieldListField.getfield, we see that the "internal" representation of this field is a list of internal representations of each element. Therefore the function FieldListField.i2m is erroneous: it does not convert the "internal" representation in "machine" representation.
• The semantics of addfield is that it expects an "internal" representation. Therefore in FieldListField.addfield the line val = self.i2m(pkt, val) can be removed. We should't need to deal with the case where val is None, because it is an invalid "internal" representation, and having None as the default value is already managed in __init__.

I also would like to suggest a much more complex change: let's take for example the following piece of code:

class X(Packet):
    fields_desc = [ FieldListField("f", [], ByteField("",0)) ]
m = X()
m.f.append(3)
m.show()
assert(repr(m) == "<X  |>")
assert(raw(m) == b"\x03")
assert(m.default_fields == {'f': [3]})
assert(m.fields == {})

This is an odd behaviour, because in this case m.f is m.default_fields['f'] which should not be changed by simple operations. A workaround is to do m.f = m.f before modifying m.f but this is not intuitive.

• First proposal: we can note that m.f is the "human" representation of the field list, and therefore there is no reason that changing this value should change the content of the packet. Therefore we define FieldListField.i2h = lambda self, pkt, x: list(x). By making a copy we make sure that modifications of m.f have no impact on m (this breaks some non-regression tests). We can even define FieldListField.i2h = lambda self, pkt, x: tuple(x) to make it immutable, but it will break more non-regression tests.
• Second proposal: use some other type than list for the "human" representation, to allow modifications of m.f with a sound behaviour. I have begun to work on such an implementation, and I am wondering if others are interested. I prefer this approach because I like when scapy is doing magic, and I can extend this approach to an implementation of HTTP where p.headers['Content-Type'] can be used to read or write a specific header...

[gpotter2]

@guedou @p-l- Any thoughts ?

[LRGH]

I thought that this issue was forgotten...
If it can help to decide whether it should be included mainstream, please find attached the additions to scapy that I am currently using in my developments.

I define FieldListFieldX and MutableList which correspond to the proposal above.
I also define PacketListFieldX which improves on the standard PacketListField.
I also define StrTermField which is useful for text-based protocols.

[guedou]

The second proposal sounds interesting. Could you submit a PR with a proposal? That will greatly simplify the review process.

[LRGH]

I can submit a PR, but before I would like to have some feedback, because there are two options, and I don't want to submit a PR for each option:

• Option 1. As in my initial proposal, it makes a modification of FieldListField, which will come with modifications of some non-regression tests.
• Option 2. As in the attached file, it adds a new field type FieldListFieldX, which will come with its own non-regression tests.

Same two options for PacketListField.

[gpotter2]

Hum, this seemed to have slipped under the radar...

• ✔️ About the main issue: between Cache fields initialization #642 and Fix race conditions on Init #2102, the bug seems to have been fixed. Note that the issue of mutable defaults was actually already discussed over Cache fields initialization #642 too. Cache fields initialization #642 was merged 6 days after this issue opened, and Fix race conditions on Init #2102 years later.

As of master:

>>> class X(Packet):
...:         fields_desc = [ FieldListField("f", [], ByteField("",0)) ]
...:
>>> m = X()
>>> m.f.append(3)
>>> m
<X  f=[3] |>
>>> m.fields
{'f': [3]}
>>> m.default_fields
{'f': []}
>>> X().fields
{'f': []}

• ✔️ PacketListField was updated separatly (see changes) to support zero-length lists with next_cls_cb.
• I'm not sure if we want to start including Text based fields within Scapy. It's probably easier to implement a custom build and dissect function when it comes to handling plain strings, because Fields aren't really made for that. There aren't much text protocols left anyways.

However, it is true that FieldListField does not need i2m. This still needs to be done. Apart from that, it seems this PR sorta resolved itself.

Any further questions/thoughts ? sorry for the ridiculous delay