Skip to content

contrib/tcpros: read field lengths in place instead of re-slicing - #5252

Open
Eapp1e wants to merge 1 commit into
secdev:masterfrom
Eapp1e:perf/tcpros-field-lengths
Open

Eapp1e wants to merge 1 commit into
secdev:masterfrom
Eapp1e:perf/tcpros-field-lengths

Conversation

@Eapp1e

@Eapp1e Eapp1e commented Oct 25, 2026

Copy link
Copy Markdown

scapy/contrib/tcpros.py parses its field lists by re-slicing the remaining buffer on every field (guess_payload_class, TCPROSHeader.pre_dissect, TCPROSBody.pre_dissect), so every field copies the rest of the buffer.

This reads the field lengths in place with struct.unpack_from() through a shared tcpros_iter_field_lengths() helper. The parsing result is unchanged; on a header of 8000 fields, TCPROS.guess_payload_class() goes from 32.7 ms to 1.8 ms.

Closes #5244

This message was written with the help of AI.

@codecov

codecov Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.45%. Comparing base (770ab0f) to head (1f4b436).

Files with missing lines Patch % Lines
scapy/contrib/tcpros.py 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5252      +/-   ##
==========================================
+ Coverage   80.41%   80.45%   +0.03%     
==========================================
  Files         376      376              
  Lines       98405    98401       -4     
==========================================
+ Hits        79137    79167      +30     
+ Misses      19268    19234      -34     
Files with missing lines Coverage Δ
scapy/contrib/tcpros.py 76.42% <86.66%> (+21.56%) ⬆️

... and 4 files with indirect coverage changes

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

guess_payload_class() and both pre_dissect() loops re-sliced the remaining
buffer for every header field, so dissecting a header with N fields copied
the whole remainder N times.

Add tcpros_iter_field_lengths(), which unpacks each length with
struct.unpack_from() and advances an offset, and use it in the three places
that walked the field list. The end-of-buffer check in
guess_payload_class() becomes an offset comparison instead of comparing the
declared length with the length of a freshly built slice.

Cover the traversal, the offset start, a 300-field header and the
conf.debug_dissector output in test/contrib/tcpros.uts.
@Eapp1e
Eapp1e force-pushed the perf/tcpros-field-lengths branch from 63dbae4 to 0772fb7 Compare October 25, 2026 14:27
@Eapp1e

Eapp1e commented Oct 25, 2026

Copy link
Copy Markdown
Author

The Codecov comment above predates the current head (0772fb73). Those two uncovered lines were the conf.debug_dissector prints in TCPROSHeader.pre_dissect, and they are now exercised by the two new cases in test/contrib/tcpros.uts (a header/body pair with debug_dissector forced on, restored in a finally):

$ python -m scapy.tools.UTscapy -t test/contrib/tcpros.uts -f text
UTscapy ended successfully

So the patch should be fully covered on re-run; the stale 86.67% is from the previous commit (1f4b436).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

contrib/tcpros: three field-list parsers re-slice the buffer per field (O(n²))

1 participant