Skip to content

fix: reject oversized telnet payload before header copy - #16404

Open
RainYuY wants to merge 1 commit into
apache:3.3from
RainYuY:3.3
Open

fix: reject oversized telnet payload before header copy#16404
RainYuY wants to merge 1 commit into
apache:3.3from
RainYuY:3.3

Conversation

@RainYuY

@RainYuY RainYuY commented Jul 31, 2026

Copy link
Copy Markdown
Member

When the server-side ExchangeCodec receives non-Dubbo input, it currently allocates and copies the entire readable buffer before the Telnet payload limit is checked. An oversized input can therefore cause an unnecessary large byte-array allocation and copy.

This change validates the readable payload size in ExchangeCodec.decode() before allocating the header buffer for non-magic server-side input.

What has changed?

  • Added an early checkPayload(channel, readable) for non-magic server-side input.
  • The validation is performed before new byte[] and buffer.readBytes(...).
  • Inputs with the Dubbo magic header continue through the existing Dubbo header/body decoding path.
  • Client-side decoding and Telnet command parsing behavior remain unchanged.
  • No cross-packet scan state or decoder structure was introduced.

Tests

Added a regression test to verify that:

  • An oversized non-magic input throws ExceedPayloadLimitException.
  • The input buffer is not consumed when validation fails.

Executed:

mvn -pl dubbo-remoting/dubbo-remoting-api \
  -DskipITs \
  -Dcheckstyle.skip \
  -Drat.skip=true \
  -Dtest=ExchangeCodecTest \
  test

@codecov-commenter

codecov-commenter commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.88%. Comparing base (3a30432) to head (63808cc).

Additional details and impacted files
@@            Coverage Diff            @@
##                3.3   #16404   +/-   ##
=========================================
  Coverage     60.87%   60.88%           
- Complexity    11766    11769    +3     
=========================================
  Files          1953     1953           
  Lines         89273    89278    +5     
  Branches      13473    13477    +4     
=========================================
+ Hits          54346    54354    +8     
+ Misses        29333    29327    -6     
- Partials       5594     5597    +3     
Flag Coverage Δ
integration-tests-java21 32.15% <20.00%> (+<0.01%) ⬆️
integration-tests-java8 32.22% <20.00%> (+0.01%) ⬆️
samples-tests-java21 32.18% <20.00%> (-0.01%) ⬇️
samples-tests-java8 29.83% <20.00%> (+0.04%) ⬆️
unit-tests-java11 59.13% <100.00%> (+0.01%) ⬆️
unit-tests-java17 58.61% <100.00%> (-0.03%) ⬇️
unit-tests-java21 58.61% <100.00%> (+<0.01%) ⬆️
unit-tests-java25 58.56% <100.00%> (+<0.01%) ⬆️
unit-tests-java8 59.14% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zrlw

zrlw commented Aug 4, 2026

Copy link
Copy Markdown
Contributor
        if (isServerSide(channel)
                && readable > 1
                && (buffer.getByte(buffer.readerIndex()) != MAGIC_HIGH
                        || buffer.getByte(buffer.readerIndex() + 1) != MAGIC_LOW)) {
            checkPayload(channel, readable);
        }

It may be sufficient to only handle cases greater than 1

@RainYuY

RainYuY commented Aug 4, 2026

Copy link
Copy Markdown
Member Author
        if (isServerSide(channel)
                && readable > 1
                && (buffer.getByte(buffer.readerIndex()) != MAGIC_HIGH
                        || buffer.getByte(buffer.readerIndex() + 1) != MAGIC_LOW)) {
            checkPayload(channel, readable);
        }

It may be sufficient to only handle cases greater than 1

I don't think this change provides any meaningful behavioral benefit. When readable == 1, checkPayload is effectively a no-op because the smallest positive payload limit is 1, and the check only rejects size > payload. The current condition is correct and keeps the zero-readable case explicit, so I'd prefer to leave it as is.

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.

3 participants