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

client: fix Agent use after free #2037

Merged
merged 3 commits into from Aug 22, 2022
Merged

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented Aug 20, 2022

Please provide enough information so that others can review your pull request:

test may fail with race problem.

==================
--- FAIL: Test_Client_Unsupported_Protocol (0.00s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Agent_Custom_Response (0.01s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Ctx_ClientHelloInfo (0.00s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Agent_RetryIf (0.00s)
    testing.go:1319: race detected during execution of test
    --- FAIL: Test_Client_Agent_Dest/enough_dest (0.00s)
        testing.go:1319: race detected during execution of test
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Agent_MaxRedirectsCount (0.01s)
    --- FAIL: Test_Client_Agent_MaxRedirectsCount/error (0.00s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Put (0.01s)
--- FAIL: Test_Client_Patch (0.01s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Agent_Struct (0.00s)
--- FAIL: Test_Client_Delete (0.00s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Post (0.00s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Head (0.00s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_UserAgent (0.01s)
    --- FAIL: Test_Client_UserAgent/default (0.01s)
        testing.go:1319: race detected during execution of test
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Agent_InsecureSkipVerify (0.01s)
    testing.go:1319: race detected during execution of test
--- FAIL: Test_Client_Agent_Timeout (0.31s)
FAIL

Explain the details for making this change. What existing problem does the pull request solve?

Use After Free:

Agent.Struct may read Agent.jsonDecoder after agent.Bytes calling agent.release

@trim21 trim21 marked this pull request as ready for review August 20, 2022 08:22
@trim21 trim21 changed the title v3(client): fix test race v3(client): fix agent use after free Aug 20, 2022
@wangjq4214
Copy link
Member

The client is being refactored and I will avoid these issues in my commit. Maybe it can be closed?

@efectn
Copy link
Member

efectn commented Aug 20, 2022

Perhaps master would be better for this PR

@wangjq4214
Copy link
Member

Perhaps master would be better for this PR

Yes, I think so, changing target branch is better.

@efectn efectn changed the base branch from v3-beta to gh-pages August 20, 2022 11:39
@efectn efectn changed the title v3(client): fix agent use after free client: fix agent use after free Aug 20, 2022
@efectn efectn changed the base branch from gh-pages to master August 20, 2022 11:40
@efectn
Copy link
Member

efectn commented Aug 20, 2022

Can you rebase 😅😅

@efectn efectn removed the v3 label Aug 20, 2022
@trim21

This comment was marked as outdated.

@trim21
Copy link
Contributor Author

trim21 commented Aug 20, 2022

done

@trim21 trim21 changed the title client: fix agent use after free client: fix Agent use after free Aug 21, 2022
@trim21 trim21 requested a review from efectn August 22, 2022 03:49
@ReneWerner87 ReneWerner87 merged commit 80a6fdc into gofiber:master Aug 22, 2022
@trim21 trim21 deleted the v3-client-race branch August 22, 2022 06:04
@trim21
Copy link
Contributor Author

trim21 commented Aug 23, 2022

can we merge this into v3 too? current flaky test is a little bit annoying

@efectn
Copy link
Member

efectn commented Aug 23, 2022

can we merge this into v3 too? current flaky test is a little bit annoying

i'll merge it asap

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

Successfully merging this pull request may close these issues.

None yet

4 participants