You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "liaoxin01 (via GitHub)" <gi...@apache.org> on 2026/04/03 16:02:19 UTC
[PR] [fix](be) Validate stream load content length before group commit [doris]
liaoxin01 opened a new pull request, #62110:
URL: https://github.com/apache/doris/pull/62110
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Group commit pre-check parsed Content-Length before the normal request validation path, so malformed values could escape the expected InvalidArgument handling.
### Release note
None
### Check List (For Author)
- Test: No completed automated test run in this environment.
- Behavior changed: Yes. Malformed Content-Length now returns a validation error instead of escaping the normal error path.
- Does this need documentation: No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "liaoxin01 (via GitHub)" <gi...@apache.org>.
liaoxin01 commented on PR #62110:
URL: https://github.com/apache/doris/pull/62110#issuecomment-4248968156
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62110:
URL: https://github.com/apache/doris/pull/62110#issuecomment-4193207206
# BE Regression && UT Coverage Report
Increment line coverage `66.67% (6/9)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62110_1c56c21ef3da9619e07a94ea88126ffb0106bf5f_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62110_1c56c21ef3da9619e07a94ea88126ffb0106bf5f_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 71.88% (26675/37108) |
| Line Coverage | 54.78% (281690/514251) |
| Region Coverage | 51.80% (233305/450419) |
| Branch Coverage | 53.28% (100891/189368) |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "liaoxin01 (via GitHub)" <gi...@apache.org>.
liaoxin01 commented on PR #62110:
URL: https://github.com/apache/doris/pull/62110#issuecomment-4248892685
/reivew
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "liaoxin01 (via GitHub)" <gi...@apache.org>.
liaoxin01 commented on PR #62110:
URL: https://github.com/apache/doris/pull/62110#issuecomment-4204308255
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #62110:
URL: https://github.com/apache/doris/pull/62110#discussion_r3033411811
##########
be/src/service/http/action/stream_load.cpp:
##########
@@ -906,9 +906,15 @@ Status StreamLoadAction::_check_wal_space(const std::string& group_commit_mode,
Status StreamLoadAction::_can_group_commit(HttpRequest* req, std::shared_ptr<StreamLoadContext> ctx,
std::string& group_commit_header,
bool& can_group_commit) {
- int64_t content_length = req->header(HttpHeaders::CONTENT_LENGTH).empty()
- ? 0
- : std::stoll(req->header(HttpHeaders::CONTENT_LENGTH));
+ int64_t content_length = 0;
+ if (!req->header(HttpHeaders::CONTENT_LENGTH).empty()) {
+ try {
+ content_length = std::stoll(req->header(HttpHeaders::CONTENT_LENGTH));
+ } catch (const std::exception& e) {
+ return Status::InvalidArgument("invalid HTTP header CONTENT_LENGTH={}: {}",
+ req->header(HttpHeaders::CONTENT_LENGTH), e.what());
Review Comment:
`req->header(HttpHeaders::CONTENT_LENGTH)` is looked up multiple times (empty check, parsing, and again in the error message). Since `HttpRequest::header()` does a map lookup each call, consider caching the header value (e.g., `const auto& content_length_str = ...`) and using that for the empty check / `stoll` / error message to avoid redundant lookups and ensure the reported value matches what was parsed.
```suggestion
const auto& content_length_str = req->header(HttpHeaders::CONTENT_LENGTH);
if (!content_length_str.empty()) {
try {
content_length = std::stoll(content_length_str);
} catch (const std::exception& e) {
return Status::InvalidArgument("invalid HTTP header CONTENT_LENGTH={}: {}",
content_length_str, e.what());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "gavinchou (via GitHub)" <gi...@apache.org>.
gavinchou merged PR #62110:
URL: https://github.com/apache/doris/pull/62110
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62110:
URL: https://github.com/apache/doris/pull/62110#issuecomment-4205118243
# BE Regression && UT Coverage Report
Increment line coverage `85.00% (17/20)` :tada:
[Increment coverage report](http://coverage.selectdb-in.cc/coverage/62110_4bd08e68d4a5eba4b11b63d726b8e8d40b4b0db8_merge/increment_report/index.html)
[Complete coverage report](http://coverage.selectdb-in.cc/coverage/62110_4bd08e68d4a5eba4b11b63d726b8e8d40b4b0db8_merge/report/index.html)
| Category | Coverage |
|-------------------|--------------------|
| Function Coverage | 73.63% (27364/37162) |
| Line Coverage | 57.25% (295212/515646) |
| Region Coverage | 54.51% (245970/451241) |
| Branch Coverage | 56.12% (106540/189829) |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "hello-stephen (via GitHub)" <gi...@apache.org>.
hello-stephen commented on PR #62110:
URL: https://github.com/apache/doris/pull/62110#issuecomment-4184064565
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR).
Please clearly describe your PR:
1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
3. What features were added. Why was this function added?
4. Which code was refactored and why was this part of the code refactored?
5. Which functions were optimized and what is the difference before and after the optimization?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #62110:
URL: https://github.com/apache/doris/pull/62110#issuecomment-4192547696
<details>
<summary>TPC-H: <b>Total hot run time: 28844 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 1c56c21ef3da9619e07a94ea88126ffb0106bf5f, data reload: false
------ Round 1 ----------------------------------
orders Doris NULL NULL 0 0 0 NULL 0 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 17732 3646 3623 3623
q2 q3 10689 859 604 604
q4 4678 465 357 357
q5 7447 1321 1140 1140
q6 184 164 140 140
q7 919 942 791 791
q8 9388 1435 1319 1319
q9 5663 5327 5241 5241
q10 6294 2044 1752 1752
q11 477 274 277 274
q12 699 406 282 282
q13 18157 2819 2160 2160
q14 277 292 252 252
q15 q16 897 864 785 785
q17 962 1028 847 847
q18 6456 5655 5503 5503
q19 1405 1261 985 985
q20 552 423 291 291
q21 4765 2502 2132 2132
q22 477 418 366 366
Total cold run time: 98118 ms
Total hot run time: 28844 ms
----- Round 2, with runtime_filter_mode=off -----
orders Doris NULL NULL 150000000 42 6422171781 NULL 22778155 NULL NULL 2023-12-26 18:27:23 2023-12-26 18:42:55 NULL utf-8 NULL NULL
============================================
q1 4671 4576 4511 4511
q2 q3 4625 4775 4096 4096
q4 2092 2162 1394 1394
q5 4954 4979 5209 4979
q6 194 167 131 131
q7 1962 1722 1588 1588
q8 3276 3031 3106 3031
q9 8213 8201 8241 8201
q10 4500 4487 4222 4222
q11 665 585 414 414
q12 670 724 498 498
q13 2765 3017 2351 2351
q14 299 323 273 273
q15 q16 754 771 670 670
q17 1287 1259 1207 1207
q18 7882 7005 6929 6929
q19 1158 1177 1127 1127
q20 2227 2184 1925 1925
q21 5934 5327 4762 4762
q22 544 494 418 418
Total cold run time: 58672 ms
Total hot run time: 52727 ms
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "dataroaring (via GitHub)" <gi...@apache.org>.
dataroaring commented on PR #62110:
URL: https://github.com/apache/doris/pull/62110#issuecomment-4192394862
run buildall
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](be) Validate stream load content length before group commit [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #62110:
URL: https://github.com/apache/doris/pull/62110#discussion_r3083857896
##########
be/src/service/http/action/stream_load.cpp:
##########
@@ -906,9 +906,16 @@ Status StreamLoadAction::_check_wal_space(const std::string& group_commit_mode,
Status StreamLoadAction::_can_group_commit(HttpRequest* req, std::shared_ptr<StreamLoadContext> ctx,
std::string& group_commit_header,
bool& can_group_commit) {
- int64_t content_length = req->header(HttpHeaders::CONTENT_LENGTH).empty()
- ? 0
- : std::stoll(req->header(HttpHeaders::CONTENT_LENGTH));
+ int64_t content_length = 0;
+ const auto& content_length_str = req->header(HttpHeaders::CONTENT_LENGTH);
Review Comment:
This validation is still bypassed when the client explicitly sends `group_commit: off_mode`. `_handle_group_commit()` returns early for `off_mode` before calling `_can_group_commit()`, so a request such as `Content-Length: -2` can still flow into `_on_header()`, store `ctx->body_bytes = -2`, and later reach `_process_put()`. In the streaming path that can construct `StreamLoadPipe(..., -2)`, which is then rejected later as an internal `_total_length < -1` error instead of an early `InvalidArgument` for bad user input. The `Content-Length` validation needs to be shared with the `off_mode` path as well, or moved to a common validation point before the early return.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org