Skip to content
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.

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

fix: hertz panic when edit ctx.Params on HandleFunc #1150

Merged
merged 10 commits into from
Jul 17, 2024

Conversation

3DRX
Copy link
Contributor

@3DRX 3DRX commented Jul 8, 2024

What type of PR is this?

fix

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo

(Optional) Translate the PR title into Chinese.

修复因用户在 HandleFunc 中修改 ctx.Params 值导致的 panic

(Optional) More detailed description for this PR(en: English/zh: Chinese).

en: This panic is happening because RequestContext.ResetWithoutConn only reset the length of Params (slice), not the array itself. Therefore, if the Params is re-assigned to another array by the user, it is possible that the capacity is not enough, leading to an out of bound array index. To fix this, we need to compare the current capacity and the expected capacity in RequestContext.ResetWithoutConn, and if they do not match, realloc Params.
zh(optional):

(Optional) Which issue(s) this PR fixes:

Fixes #1149

(Optional) The PR that updates user documentation:

@3DRX 3DRX requested review from a team as code owners July 8, 2024 17:26
@CLAassistant
Copy link

CLAassistant commented Jul 8, 2024

CLA assistant check
All committers have signed the CLA.

pkg/app/context.go Outdated Show resolved Hide resolved
pkg/app/context.go Outdated Show resolved Hide resolved
pkg/app/context.go Outdated Show resolved Hide resolved
Duslia
Duslia previously approved these changes Jul 9, 2024
Duslia
Duslia previously approved these changes Jul 10, 2024
@Duslia
Copy link
Member

Duslia commented Jul 10, 2024

@3DRX
Copy link
Contributor Author

3DRX commented Jul 10, 2024

把 reset 里的逻辑挪到这里判断吧 https://github.com/cloudwego/hertz/blob/develop/pkg/route/engine.go#L753。这里是离使用最近的一个地方,怕还有其他的地方有一些错误使用,导致这里 panic 了。

改了,由于这个逻辑移动到了 RequestContext 外部,因此添加了 GetParamsCount 函数来访问 Params 原有的长度。

@3DRX 3DRX requested review from Duslia, welkeyever and wzekin July 10, 2024 15:53
pkg/route/engine.go Outdated Show resolved Hide resolved
pkg/route/engine.go Outdated Show resolved Hide resolved
Duslia
Duslia previously approved these changes Jul 16, 2024
pkg/route/engine.go Outdated Show resolved Hide resolved
Copy link
Member

@welkeyever welkeyever left a comment

Choose a reason for hiding this comment

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

LGTM,thanks~

@welkeyever welkeyever merged commit 764df4d into cloudwego:develop Jul 17, 2024
14 checks passed
@3DRX 3DRX deleted the fix/user_edit_params_cause_panic branch July 17, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

hertz panic when edit ctx.Params on HandleFunc
5 participants