Skip to content

Conversation

@ryanzryu
Copy link
Contributor

@ryanzryu ryanzryu commented Nov 2, 2023

Proposed changes

Thrift server reads the first four bytes of a packet as the packet size, and without setting the string_limit parameter, it will directly request the memory corresponding to the packet size represented by the first four bytes. When receiving an error or large packet, it is easy to encounter OOM.
image

Doris currently does not set the string_limit parameter, so when receiving a corrupted packet, it is easy to encounter OOM.

We can send multiple HTTP packets to the Thrift port simultaneously to reproduce:
#/bin/bash
for (( i=0; i< 20 ;i++))
do
curl -X POST -d'{test}' "http://127.0.0.1:9020/" &
done

This PR add string_limit parameter for be and fe thrift server

I have submitted another identical PR to the 1.2 branch.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2023

clang-tidy review says "All clean, LGTM! 👍"

@morningman
Copy link
Contributor

run buildall

Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Mar 15, 2024
@github-actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 34.94% (8573/24533)
Line Coverage: 26.65% (69480/260761)
Region Coverage: 25.94% (36096/139174)
Branch Coverage: 22.87% (18430/80572)
Coverage Report: http://coverage.selectdb-in.cc/coverage/6171c1d5ae9b4caf0146c966e2bbd57468021559_6171c1d5ae9b4caf0146c966e2bbd57468021559/report/index.html

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

Labels

approved Indicates a PR has been approved by one committer. dev/1.2.8-merged reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants