Refactor record padding handling to eliminate middle padding pattern

[julek-wolfssl]

Move padSz index advancement from individual message handlers to a
single location at the end of record processing in ProcessReply.
Previously, each handler (DoFinished, DoApplicationData, DoAlert,
DoCertificate, DoServerHello, etc.) advanced the index by padSz,
which then had to be corrected when processing multiple messages
in one record. This "middle padding" pattern was error-prone.

Now curSize is reduced by padSz after decryption/verification so
it reflects content size only. Message handlers advance the index
by content size only. padSz is added once when the record is fully
processed. The correction code for multi-message records is removed.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Refactors TLS record padding handling so padSz is accounted for once per record (in ProcessReply/DoProcessReplyEx) instead of being applied and “corrected” within each individual message handler.

Changes:

• Removes per-message padSz index advancement across TLS 1.3 and (D)TLS message handlers.
• Adjusts DoProcessReplyEx to treat curSize as content-only by subtracting padSz, and adds padSz once when the record is fully processed.
• Updates multi-message record flow to eliminate the prior “middle padding” correction, and restricts buffer shrinking during multi-message processing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/tls13.c	Removes per-handler padSz advancement so TLS 1.3 handlers advance by content length only.
src/internal.c	Centralizes padSz accounting in DoProcessReplyEx, updating message parsing logic and removing old padding correction paths.

Comments suppressed due to low confidence (4)

src/internal.c:1

• Subtracting padSz from ssl->curSize without first verifying curSize >= padSz can underflow on malformed/truncated records and corrupt subsequent parsing/state. Add a guard (e.g., if ssl->curSize < ssl->keys.padSz return a length/buffer error) before performing the subtraction.
  src/internal.c:1
• Advancing inputBuffer.idx by padSz has no bounds validation. If padding/tag bytes are missing (corrupted/truncated input), idx can jump beyond the actual buffered record data and leave the connection state inconsistent. Add a bounds check before incrementing (e.g., ensure idx + padSz does not exceed the available buffered data for the current record) and fail with a parse error if it would.
  src/internal.c:1
• inputLength is reduced by padSz without checking that inputLength >= padSz. When the remaining buffered bytes are smaller than padSz, this will underflow (likely to a very large word32) and can cause oversized copies/allocations or out-of-bounds reads later. Add an explicit check before subtracting and error out when insufficient bytes remain.
  src/internal.c:1
• The parameter size is used (if (size)), so (void)size; is redundant and can be removed to reduce confusion.

---

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

[julek-wolfssl]

Retest this please history lost........

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (3)

src/internal.c:1

• ssl->curSize is an unsigned type; subtracting padSz can underflow if padSz > curSize (e.g., malformed/short records or if padSz is unexpectedly large), which would inflate curSize and break subsequent bounds checks (including the new curStartIdx + curSize calculations). Make this subtraction conditional on ssl->curSize >= ssl->keys.padSz (return an error like BUFFER_E/LENGTH_ERROR otherwise), and avoid truncating padSz to word16 when comparing/subtracting.
  src/internal.c:1
• This DTLS 'drop record outside receiving window' path adds ssl->keys.padSz unconditionally, but curSize is only reduced by padSz when IsEncryptionOn(ssl, 0) is true. If encryption is off (or if padSz is stale from a prior encrypted record), this can skip too far and desynchronize parsing. Add padSz here only when the current record is encrypted (or ensure padSz is reset to 0 for plaintext records before this path).
  src/internal.c:1
• After advancing inputBuffer.idx by padSz, the code only checks >= length (and returns), but does not prevent idx from becoming > length. Leaving inputBuffer.idx past the buffer end can violate invariants for subsequent calls/paths. Add an explicit if (idx > length) return BUFFER_E; (or equivalent) immediately after the idx += padSz increment (and similarly in other padSz-advance early-return paths).

---

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

[dgarske]

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 4 total — 3 posted, 2 skipped

Posted findings

• [Medium] Missing defensive guard before curSize -= padSz could allow word16 underflow — src/internal.c:23184-23185
• [Medium] Callers of message handlers still pass inputBuffer.length as totalSz, not content boundary — src/internal.c:23286-23289,23306-23309,23343-23346,23360-23362
• [Medium] No dedicated test for multi-message encrypted record processing after refactoring — src/internal.c:23654-23695

Skipped findings

• [Low] Truncation cast of word32 padSz to word16 suppresses potential warnings
• [Medium] Callers of message handlers still pass inputBuffer.length as totalSz, not content boundary

Review generated by Skoll via openclaw

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[julek-wolfssl]

Failures unrelated to PR.

[JacobBarthelmeh]

Retest this please Jenkins

[JacobBarthelmeh]

This appears to be a good refactor. I need help on closer review of DTLS 1.3 changes though. @rizlik could you provide an additional review?

[douzzer]

retest this please
(results lost)

[rizlik]

Very nice!
Can you add a github action that install python script dependencies and run the test? My fear is that this test might be skipped because of the missing dependency and the multimessage is left untested in our CI.

[julek-wolfssl]

@rizlik added python setup to relevant workflows.

[github-actions]

MemBrowse Memory Report

gcc-arm-cortex-m4

• FLASH: .text -448 B (-0.2%, 197,569 B / 262,144 B, total: 75% used)

gcc-arm-cortex-m4-tls12

• FLASH: .text -384 B (-0.3%, 121,053 B / 262,144 B, total: 46% used)
  No memory changes detected for:
• gcc-arm-cortex-m4-baremetal
• gcc-arm-cortex-m4-min-ecc