Skip to content
This repository has been archived by the owner on Jun 23, 2022. It is now read-only.

fix(http): fix message buffers size check #662

Merged
merged 3 commits into from
Nov 11, 2020

Conversation

hycdong
Copy link
Contributor

@hycdong hycdong commented Nov 10, 2020

In previous pr #593, http server can parse content-type of HTTP request. The incoming http request will parse into message_ex structure, msg->buffers[3] indicate the content-type, the msg->buffers will be resized as 4. When parsing message into http _request, the size of buffers will be checked. However, function parse still consider the right size of buffers is 3, it will reject all requests. This pull request fix it, add BUFFER_SIZE and update related unit tests.

@hycdong hycdong marked this pull request as ready for review November 10, 2020 05:20
@hycdong hycdong added the type/bug-fix This PR fixes a bug. label Nov 10, 2020
@neverchanje neverchanje changed the title fix(http): fix buffer size check fix(http): fix message buffers size check Nov 10, 2020
@@ -38,6 +38,9 @@ namespace dsn {

DEFINE_CUSTOMIZED_ID(network_header_format, NET_HDR_HTTP)

// Number of blobs that a message_ex contains.
#define HTTP_MSG_BUFFERS_NUM 4
Copy link
Contributor

Choose a reason for hiding this comment

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

I think const uint8_t is better here

Copy link
Contributor

Choose a reason for hiding this comment

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

Using constexpr or macro is basically the same, but using const is not a good practice, because the var is loaded in runtime.

@neverchanje neverchanje merged commit 81b1005 into XiaoMi:master Nov 11, 2020
@hycdong hycdong deleted the fix_http_bug_1 branch November 11, 2020 08:32
zhangyifan27 pushed a commit to zhangyifan27/rdsn that referenced this pull request Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/bug-fix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants