Skip to content

[enhancement] Enable clear_cache() to spread upward in the packet tree #4707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
alxroyer-thales opened this issue Mar 28, 2025 · 4 comments
Open

[enhancement] Enable clear_cache() to spread upward in the packet tree #4707

alxroyer-thales opened this issue Mar 28, 2025 · 4 comments

Comments

@alxroyer-thales
Copy link

alxroyer-thales commented Mar 28, 2025 •
edited
Loading

Enable clear_cache() to spread upward in the packet tree (underlayer and parent packets),
and not downward as currently (fields and payload).
@alxroyer-thales
Copy link
Author

Provides a straight forward implementation for #4706.

Also improves performance in case of large packet trees with many packet fields (see #4705).

@alxroyer-thales
Copy link
Author

Successfully implemented in a Scapy wrapper.

I'll provide a PR soon to propose the integration of it directly in Scapy implementation.

@alxroyer-thales alxroyer-thales mentioned this issue Mar 28, 2025
Open
@gpotter2
Copy link
Member

gpotter2 commented Apr 6, 2025 •
edited
Loading

Hm. Thanks for the suggestion. A bit unsure, but it feels like a pretty decent idea to me. I agree that the current behavior of caching can be hard to grasp.

@p-l- @guedou What do you think?

@alxroyer-thales
Copy link
Author

In order to ensure both 1) improve performance, and 2) avoid breaking the API, we could add parameters to clear_cache() to provide both behaviours:

alxroyer-thales added a commit to ThalesGroup/scapy that referenced this issue Apr 18, 2025
@alxroyer-thales
Report [email protected] optimizations in [email protected] (secdev#4705, secdev#…
822246c 
…4706, secdev#4707)

- secdev#4705: Performance issues with nested `PacketField`s:
    - Cache saved in `Packet.self_build()`.
    - `Packet`s marked as explicit systematically in `Packet.build()`
      to avoid a useless clone in `do_build()`.
    - `Packet.do_init_cached_fields()` optimized:
        - `init_fields` parameter added to `Packet.init_fields()` and
          `do_init_cached_fields()`.
        - Misc: `do_init_cached_fields()` fixed for list field copies.
    - `Packet.prepare_cached_fields()` optimized:
        - Default fields not copied anymore.
    - `Packet.copy()` and `clone_with()` optimized with `_fast_copy()`:
        - Avoid instantiating *ref fields* with default instantiation.
        - `Packet.copy_fields_dict()`
          optimized with new `use_fields_already_set` parameter
          and reworked as an inner method to work directly
          on the `clone` instance.
        - Misc: `_T` variable type at the module level made local
          and renamed as `_VarDictFieldType` in `_fast_copy()`.
    - `Packet.setfieldval()` optimized:
        - New value discarded if unchanged.:
    - `_PacketField.m2i()` optimized:
        - Avoid useless copies of fields with default instance.
        - Then `dissect()` is used on it.
- secdev#4707: [enhancement] Enable clear_cache() to spread upward
  in the packet tree:
    - `Packet.clear_cache()` reworked.
- secdev#4706: Force cache reset as soon as the payload or a packet field
  is modified:
    - `Packet.clear_cache()` (reworked as per secdev#4707) called in
      `setfieldval()`, `delfieldval()`, `add_payload()` and
      `remove_payload()`.
    - `Packet.raw_packet_cache_fields` now useless, thus removed.
    - Same for `Packet._raw_packet_cache_field_value()`.
    - `Packet.self_build()` simplified by the way.
    - `Packet.no_cache` flag added to avoid setting `raw_packet_cache`
      when specified, used in `Packet.self_build()` and `do_dissect()`.
alxroyer-thales added a commit to ThalesGroup/scapy that referenced this issue Apr 18, 2025
@alxroyer-thales
Restore default clear_cache() behaviour (secdev#4707)
e8a1b85 
- `upwards` (`False` by default) and `downwards` (`True` by default)
  parameters added to `clear_cache()`.
- Set to `upwards=True` and `downwards=False` in `setfieldval()`,
  `delfieldval()`, `add_payload()` and `remove_payload()`.
- Former implementation restored for `downwards=True`.
alxroyer-thales added a commit to ThalesGroup/scapy that referenced this issue Apr 23, 2025
@alxroyer-thales
Improve 'auto_clear_cache.uts' (secdev#4706, secdev#4707)
cc2572d 
- Set step numbers.
alxroyer-thales added a commit to ThalesGroup/scapy that referenced this issue Apr 23, 2025
@alxroyer-thales
Report [email protected] optimizations in [email protected] (secdev#4705, secdev#…
af50227 
…4706, secdev#4707)

- secdev#4705: Performance issues with nested `PacketField`s:
    - Cache saved in `Packet.self_build()`.
    - `Packet`s marked as explicit systematically in `Packet.build()`
      to avoid a useless clone in `do_build()`.
    - `Packet.do_init_cached_fields()` optimized:
        - `init_fields` parameter added to `Packet.init_fields()` and
          `do_init_cached_fields()`.
        - Misc: `do_init_cached_fields()` fixed for list field copies.
    - `Packet.prepare_cached_fields()` optimized:
        - Default fields not copied anymore.
    - `Packet.copy()` and `clone_with()` optimized with `_fast_copy()`:
        - Avoid instantiating *ref fields* with default instantiation.
        - `Packet.copy_fields_dict()`
          optimized with new `use_fields_already_set` parameter
          and reworked as an inner method to work directly
          on the `clone` instance.
        - Misc: `_T` variable type at the module level made local
          and renamed as `_VarDictFieldType` in `_fast_copy()`.
    - `Packet.setfieldval()` optimized:
        - New value discarded if unchanged.:
    - `_PacketField.m2i()` optimized:
        - Avoid useless copies of fields with default instance.
        - Then `dissect()` is used on it.
- secdev#4707: [enhancement] Enable clear_cache() to spread upward
  in the packet tree:
    - `Packet.clear_cache()` reworked.
- secdev#4706: Force cache reset as soon as the payload or a packet field
  is modified:
    - `Packet.clear_cache()` (reworked as per secdev#4707) called in
      `setfieldval()`, `delfieldval()`, `add_payload()` and
      `remove_payload()`.
    - `Packet.raw_packet_cache_fields` now useless, thus removed.
    - Same for `Packet._raw_packet_cache_field_value()`.
    - `Packet.self_build()` simplified by the way.
    - `Packet.no_cache` flag added to avoid setting `raw_packet_cache`
      when specified, used in `Packet.self_build()` and `do_dissect()`.
alxroyer-thales added a commit to ThalesGroup/scapy that referenced this issue Apr 23, 2025
@alxroyer-thales
Restore default clear_cache() behaviour (secdev#4707)
35877da 
- `upwards` (`False` by default) and `downwards` (`True` by default)
  parameters added to `clear_cache()`.
- Set to `upwards=True` and `downwards=False` in `setfieldval()`,
  `delfieldval()`, `add_payload()` and `remove_payload()`.
- Former implementation restored for `downwards=True`.
alxroyer-thales added a commit to ThalesGroup/scapy that referenced this issue Apr 23, 2025
@alxroyer-thales
Fix parent not set when building default packet fields (secdev#4706, s…
33aeae4 
…ecdev#4707)

- `Packet._ensure_parent_of()` added.
- Called in `do_init_cached_fields()`.
@alxroyer-thales alxroyer-thales mentioned this issue Apr 23, 2025
Draft
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@gpotter2 @alxroyer-thales