Skip to content

fields: preserve containers when copying fields#5009

Open
T3pp31 wants to merge 3 commits into
secdev:masterfrom
T3pp31:fix-fieldcontainer-copy
Open

fields: preserve containers when copying fields#5009
T3pp31 wants to merge 3 commits into
secdev:masterfrom
T3pp31:fix-fieldcontainer-copy

Conversation

@T3pp31

@T3pp31 T3pp31 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix copying of _FieldContainer instances so wrappers such as Emph remain wrappers instead of being replaced by their contained field.

The root cause was that _FieldContainer delegated missing attributes to fld, so container.copy() resolved to container.fld.copy() when the container class did not define its own copy() method. This also affected packet field copying when fields_desc references another packet class and overrides a copied field default.

Fixes #4657.

Changes

  • Add _FieldContainer.copy() to copy the container instance and its stored attributes.
  • Delegate unsupported attribute assignment to the contained field, matching the existing read delegation needed by field default replacement.
  • Add a regression test for direct Emph(...).copy() and the packet-metaclass copy path from the issue.

Validation

  • python3 -m scapy.tools.UTscapy -t test/fields.uts -N -qq -f xUnit -o /tmp/scapy-fields.xml
  • /tmp/scapy-flake8-venv/bin/flake8 scapy/

Note: tox is not installed in this local environment, so flake8 was run through a temporary venv using the repository-pinned flake8<6.0.0 version.

@T3pp31 T3pp31 force-pushed the fix-fieldcontainer-copy branch from eac2ff2 to 159afb7 Compare June 1, 2026 23:05
@T3pp31 T3pp31 marked this pull request as ready for review June 1, 2026 23:08
@codecov

codecov Bot commented Jun 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.04%. Comparing base (66ef96a) to head (159afb7).
⚠️ Report is 48 commits behind head on master.

Files with missing lines Patch % Lines
scapy/fields.py 83.33% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5009      +/-   ##
==========================================
- Coverage   80.31%   80.04%   -0.28%     
==========================================
  Files         381      384       +3     
  Lines       93630    95658    +2028     
==========================================
+ Hits        75202    76565    +1363     
- Misses      18428    19093     +665     
Files with missing lines Coverage Δ
scapy/fields.py 92.60% <83.33%> (-0.50%) ⬇️

... and 62 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a longstanding copy bug for _FieldContainer wrappers (e.g., Emph) so that copying preserves the container type rather than collapsing to the wrapped field—particularly impacting Packet_metaclass default-replacement when fields_desc references another packet (issue #4657).

Changes:

  • Add _FieldContainer.copy() to return a copied container instance (including copying its stored attributes/slots).
  • Add _FieldContainer.__setattr__() to delegate unsupported attribute assignment to the wrapped field (mirroring existing read delegation).
  • Add regression tests covering Emph(...).copy() and the packet-metaclass copy/default override path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
scapy/fields.py Implements _FieldContainer.copy() and adds write-delegation via __setattr__ to preserve wrappers during copy/default replacement.
test/fields.uts Adds regression coverage for direct container .copy() and packet metaclass field-copy behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scapy/fields.py
Comment on lines +352 to +361
def __setattr__(self, attr, value):
# type: (str, Any) -> None
try:
object.__setattr__(self, attr, value)
except AttributeError as ex:
try:
fld = object.__getattribute__(self, "fld")
except AttributeError:
raise ex
setattr(fld, attr, value)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — __setattr__ now checks whether attr is defined on the container class (via the MRO __dict__) before delegating to the wrapped field. Read-only descriptors like MultipleTypeField.fld will raise the original AttributeError instead of being forwarded to the selected underlying field.

Comment thread scapy/fields.py
"""
__slots__ = ["fld"]

def copy(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this function does looks very funky to me. Couldn't we just call the copy() of the sub field?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point — I've simplified it significantly.

We can't just return self.fld.copy() though: that was the original bug. Without an explicit copy() on _FieldContainer, the call goes through __getattr__ and ends up as self.fld.copy(), which returns a bare ByteField instead of an Emph wrapper (see #4657).

The new implementation is much shorter: create a new container via __new__, iterate over __slots__, and call .copy() on slot values where available. This keeps the wrapper type while still deep-copying the contained field.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you commenting with AI?

T3pp31 and others added 2 commits June 26, 2026 16:28
Address review feedback on PR secdev#5009:

- Replace the MRO-based copy implementation with a simpler approach that
  creates a new container and copies each __slots__ entry, calling copy()
  on contained fields where available. This preserves the wrapper type
  (the bug was __getattr__ delegating copy to the wrapped field).
- Guard __setattr__ delegation when the attribute is defined on the
  container class (e.g. MultipleTypeField's read-only fld property).

Co-authored-by: Teppei.F <T3pp31@users.noreply.github.com>
@cursor cursor Bot force-pushed the fix-fieldcontainer-copy branch from 159afb7 to 5443e61 Compare June 26, 2026 16:29
[Conversation comment]
Thanks for the review! I've pushed an update addressing both comments:

- copy(): Simplified the implementation. The issue wasn't that we shouldn't call
  the sub-field's copy() at all — it's that without an explicit
  _FieldContainer.copy(), container.copy() was resolved via __getattr__ and
  returned only container.fld.copy() (a plain ByteField instead of Emph). The
  new version creates a new container instance and copies each __slots__ entry,
  calling .copy() on values that support it (e.g. the wrapped field).

- __setattr__: Added a guard so delegation only happens for attributes that
  aren't defined on the container class. Assignments like mtf.fld = ... on
  MultipleTypeField now correctly raise AttributeError instead of silently
  mutating the selected underlying field.

Rebased onto current master. All test/fields.uts tests pass, including the
new regression test.

[Reply to gpotter2 on copy()]
Good point — I've simplified it significantly.

We can't just return self.fld.copy() though: that was the original bug. Without
an explicit copy() on _FieldContainer, the call goes through __getattr__ and
ends up as self.fld.copy(), which returns a bare ByteField instead of an Emph
wrapper (see secdev#4657).

The new implementation is much shorter: create a new container via __new__,
iterate over __slots__, and call .copy() on slot values where available. This
keeps the wrapper type while still deep-copying the contained field.

[Reply to Copilot on __setattr__]
Fixed — __setattr__ now checks whether attr is defined on the container class
(via the MRO __dict__) before delegating to the wrapped field. Read-only
descriptors like MultipleTypeField.fld will raise the original AttributeError
instead of being forwarded to the selected underlying field.

Co-authored-by: Teppei.F <T3pp31@users.noreply.github.com>
@T3pp31

T3pp31 commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I've pushed an update addressing both comments:

  • copy(): Simplified the implementation. The issue wasn't that we shouldn't call the sub-field's copy() at all — it's that without an explicit _FieldContainer.copy(), container.copy() was resolved via __getattr__ and returned only container.fld.copy() (a plain ByteField instead of Emph). The new version creates a new container instance and copies each __slots__ entry, calling .copy() on values that support it (e.g. the wrapped field).

  • __setattr__: Added a guard so delegation only happens for attributes that aren't defined on the container class. Assignments like mtf.fld = ... on MultipleTypeField now correctly raise AttributeError instead of silently mutating the selected underlying field.

Rebased onto current master. All test/fields.uts tests pass, including the new regression test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unexpected behavior when copying FieldContainer

4 participants