Skip to content

Fix TCP_USER_TIMEOUT overflow#6711

Open
yzfeng2020 wants to merge 4 commits intoline:mainfrom
yzfeng2020:fix/tcp-user-timeout-overflow
Open

Fix TCP_USER_TIMEOUT overflow#6711
yzfeng2020 wants to merge 4 commits intoline:mainfrom
yzfeng2020:fix/tcp-user-timeout-overflow

Conversation

@yzfeng2020
Copy link
Copy Markdown
Contributor

@yzfeng2020 yzfeng2020 commented Apr 7, 2026

Motivation:

ChannelUtil.applyDefaultChannelOptions() overflows when idleTimeoutMillis is near Long.MAX_VALUE. The expression idleTimeoutMillis + TCP_USER_TIMEOUT_BUFFER_MILLIS (where the buffer is 5,000)
wraps the long type to a large negative number, and Ints.saturatedCast() then clamps it to Integer.MIN_VALUE (-2,147,483,648).

Linux rejects this negative value for TCP_USER_TIMEOUT via setsockopt() with EINVAL. Starting with Netty 4.1.131+, channel option values are validated at set time and throw exceptions
instead of being silently logged internally (see netty/netty#15896). This means the overflow now causes a hard failure rather than a silent misconfiguration.

Modifications:

  • Use LongMath.saturatedAdd to prevent the long overflow before passing the result to Ints.saturatedCast:
    final int tcpUserTimeoutMillis =
            Ints.saturatedCast(LongMath.saturatedAdd(idleTimeoutMillis,
                                                     TCP_USER_TIMEOUT_BUFFER_MILLIS));
  • Add a regression test that passes Long.MAX_VALUE as idleTimeoutMillis and asserts the result is Integer.MAX_VALUE.

Result:

  • Long.MAX_VALUE idle timeout no longer overflows — produces Integer.MAX_VALUE (~24.8 day timeout) instead of Integer.MIN_VALUE (rejected by Linux).
  • For normal values (e.g., 2000), Math.min is a no-op and behavior is unchanged.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 7, 2026

📝 Walkthrough

Walkthrough

Use overflow-safe addition when computing TCP_USER_TIMEOUT: LongMath.saturatedAdd(idleTimeoutMillis, TCP_USER_TIMEOUT_BUFFER_MILLIS) before Ints.saturatedCast; added a parameterized test asserting the TCP user timeout clamps to Integer.MAX_VALUE when maxIdleTimeout is Long.MAX_VALUE. (≤50 words)

Changes

Cohort / File(s) Summary
TCP User Timeout Overflow Fix
core/src/main/java/com/linecorp/armeria/internal/common/util/ChannelUtil.java
Replace direct idleTimeoutMillis + TCP_USER_TIMEOUT_BUFFER_MILLIS with LongMath.saturatedAdd(...) and then Ints.saturatedCast to avoid intermediate long overflow when deriving TCP_USER_TIMEOUT.
Test Coverage for Overflow Scenario
core/src/test/java/com/linecorp/armeria/internal/common/util/ChannelUtilTest.java
Add parameterized test tcpUserTimeoutWithMaxIdleTimeout to verify the computed TCP user timeout is clamped to Integer.MAX_VALUE when maxIdleTimeout is Long.MAX_VALUE.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested labels

defect

Suggested reviewers

  • trustin
  • ikhoon
  • minwoox

Poem

🐰 I nudged the sums with gentle care,
So longs won't tumble unaware.
A buffered hop,
No overflow flop,
Now TCP naps safe in my lair 🥕🛠️

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies the primary fix: preventing TCP_USER_TIMEOUT overflow in ChannelUtil, which is the main change in the changeset.
Description check ✅ Passed The pull request description clearly explains the overflow bug in TCP_USER_TIMEOUT calculation, the root cause, the fix using LongMath.saturatedAdd, and the expected behavior improvement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…idle timeouts

When idleTimeoutMillis is near Long.MAX_VALUE, the addition of
TCP_USER_TIMEOUT_BUFFER_MILLIS (5000) overflowed the long type,
producing a negative value that Linux rejects with EINVAL.

Clamp idleTimeoutMillis before addition so the sum saturates at
Long.MAX_VALUE instead of wrapping. Ints.saturatedCast then
correctly yields Integer.MAX_VALUE (~24.8 day timeout).
@yzfeng2020 yzfeng2020 force-pushed the fix/tcp-user-timeout-overflow branch from 8928e3e to b1780e5 Compare April 7, 2026 22:41
@yzfeng2020 yzfeng2020 changed the title Fix/tcp user timeout overflow Fix TCP_USER_TIMEOUT int overflow Apr 7, 2026
…T calculation

Replace manual Math.min clamp with Guava's LongMath.saturatedAdd,
which is already used throughout the codebase for the same pattern.
Remove redundant test comment.

Co-authored-by: Isaac
@yzfeng2020 yzfeng2020 changed the title Fix TCP_USER_TIMEOUT int overflow Fix TCP_USER_TIMEOUT overflow Apr 7, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.00%. Comparing base (8150425) to head (94aae8d).
⚠️ Report is 408 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6711      +/-   ##
============================================
- Coverage     74.46%   74.00%   -0.46%     
- Complexity    22234    24118    +1884     
============================================
  Files          1963     2176     +213     
  Lines         82437    90501    +8064     
  Branches      10764    11859    +1095     
============================================
+ Hits          61385    66975    +5590     
- Misses        15918    17906    +1988     
- Partials       5134     5620     +486     

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Copy Markdown
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍

@jrhee17 jrhee17 added the defect label Apr 8, 2026
@jrhee17 jrhee17 added this to the 1.39.0 milestone Apr 8, 2026
Copy link
Copy Markdown
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Nice catch! 🚀🚀

Copy link
Copy Markdown
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants