Skip to content

rpc: add SetWebsocketReadLimit in Server #32279

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
Merged
merged 6 commits into from
Aug 19, 2025
Merged

rpc: add SetWebsocketReadLimit in Server #32279

merged 6 commits into from
Aug 19, 2025

Conversation

yzang2019
Copy link
Contributor

@yzang2019 yzang2019 commented Jul 25, 2025 •
edited
Loading

TLDR:

  • Exposing the public method to setReadLimits for Websocket RPC to prevent OOM.

Context:
Current, Geth Server is using a default 32MB max read limit (message size) for websocket, which is prune to being attacked for OOM. Any one can easily launch a client to send a bunch of concurrent large request to cause the node to crash for OOM. One example of such script that can easily crash a Geth node running websocket server is like this:
https://gist.githubusercontent.com/DeltaXV/b64d221e342e9c1ec6c99c1ab8201544/raw/ec830979ac9a707d98f40dfcc0ce918fc8fb9057/poc.go

@yzang2019 yzang2019 requested a review from fjl as a code owner July 25, 2025 15:40
@yzang2019
Copy link
Contributor Author

Please help take a look, thanks!

@yzang2019 yzang2019 mentioned this pull request Jul 25, 2025
Merged
@rjl493456442
Copy link
Member

--- FAIL: TestServerSetReadLimits (0.01s)
--- FAIL: TestServerSetReadLimits/medium_limit_with_large_request_-_should_fail (0.00s)
server_test.go:284: expected 'message too big' error, got: write tcp 127.0.0.1:39864->127.0.0.1:42233: write: connection reset by peer'

Tests are failing

@rjl493456442
Copy link
Member

Current, Geth Server is using a default 32MB max read limit (message size) for websocket, which is prune to being attacked for OOM. Any one can easily launch a client to send a bunch of concurrent large request to cause the node to crash for OOM. One example of such script that can easily crash a Geth node running websocket server is like this:

It's true. Geth always assumes the instance should be protected by node operator for serving the RPC/Websocket requests. But I agree we should provide the ways for operators to apply the configurations.

@yzang2019
Copy link
Contributor Author

--- FAIL: TestServerSetReadLimits (0.01s) --- FAIL: TestServerSetReadLimits/medium_limit_with_large_request_-_should_fail (0.00s) server_test.go:284: expected 'message too big' error, got: write tcp 127.0.0.1:39864->127.0.0.1:42233: write: connection reset by peer'

Tests are failing

I think it was failing because of 1.24 tests were timing out, let me try running the test again

@rjl493456442 rjl493456442 changed the title Expose SetReadLimits for Websocket server to prevent OOM rpc: expose SetReadLimits for Websocket server Jul 31, 2025
@fjl fjl self-assigned this Aug 5, 2025
@yzang2019
Copy link
Contributor Author

How can I proceed next to rerun the failed test? I believe there were some flaky tests elsewhere that's not introduced by this change

@fjl fjl changed the title rpc: expose SetReadLimits for Websocket server rpc: add SetWebsocketReadLimit in Server Aug 8, 2025
fjl
fjl previously approved these changes Aug 8, 2025
View reviewed changes
@fjl fjl added this to the 1.16.3 milestone Aug 8, 2025
@yzang2019
Copy link
Contributor Author

Looks like the newly added unit test might be too heavy to run on github, which cause the test to timeout in 1.24, let me see if I can simplify the test a bit

@yzang2019
Merge branch 'master' into yzang/expose-readerlimit
9cd4ede 
* master: (57 commits)
  core/vm: fix EIP-7823 modexp input length check (ethereum#32363)
  rlp: remove workaround for Value.Bytes (ethereum#32433)
  consensus/misc/eip4844: use blob parameters of current header (ethereum#32424)
  crypto/bn256: refactor to use bitutil.TestBytes (ethereum#32435)
  core/vm: refactor to use bitutil.TestBytes (ethereum#32434)
  cmd/evm: use PathScheme in blockrunner (ethereum#32444)
  trie, core/state: add the transition tree (verkle transition part 2) (ethereum#32366)
  build: remove unused functions (ethereum#32393)
  crypto/secp256k1: use ReadBits from common/math (ethereum#32430)
  build: upgrade -dlgo version to Go 1.25.0 (ethereum#32412)
  .github: upgrade workflows to Go 1.25 (ethereum#32425)
  p2p: refactor to use time.Now().UnixMilli() in golang std lib (ethereum#32402)
  eth/syncer: fix typo (ethereum#32427)
  eth/tracers: Adds codeHash to prestateTracer's response (ethereum#32391)
  rlp: optimize intsize (ethereum#32421)
  node: remove unused err var (ethereum#32398)
  eth: abort `requiredBlocks` check if peer handler terminated (ethereum#32413)
  cmd: fix inconsistent function name in comment (ethereum#32411)
  trie: refactor to use slices.Concat (ethereum#32401)
  consensus: fix ambiguous invalid gas limit error (ethereum#32405)
  ...
@yzang2019
Fix unit test
539ff2a
@yzang2019
Copy link
Contributor Author

@fjl May I get another review? I've simplified the test cases, unit tests are all passing now

@yzang2019 yzang2019 requested a review from fjl August 17, 2025 22:40
@rjl493456442 rjl493456442 merged commit d93f820 into ethereum:master Aug 19, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rjl493456442 rjl493456442 rjl493456442 approved these changes

@fjl fjl Awaiting requested review from fjl fjl is a code owner

Assignees

@fjl fjl

Labels
None yet
Projects
None yet
Milestone
1.16.3
Development

Successfully merging this pull request may close these issues.
3 participants
@yzang2019 @rjl493456442 @fjl