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(context): verify URL is Non-nil in initQueryCache() #3969

Merged
merged 1 commit into from
May 19, 2024

Conversation

adrianosela
Copy link
Contributor

Verify URL is Non-nil in initQueryCache()

When using a test context (as returned by gin.CreateTestContext(w)) it is possible that not only the Request object is nil, but also the URL object may be nil.

Ran into this in the wild in unit tests. Stack trace:

1. --- FAIL: Test_ListOrgs (1.09s)
2. panic: runtime error: invalid memory address or nil pointer dereference [recovered]
3.     panic: runtime error: invalid memory address or nil pointer dereference
4. [signal SIGSEGV: segmentation violation code=0x1 addr=0x60 pc=0x6e51ce]
5.  
6. goroutine 124 [running]:
7. testing.tRunner.func1.2({0x206ad60, 0x3dd8c30})
8.     /opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1631 +0x24a
9. testing.tRunner.func1()
10.     /opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1634 +0x377
11. panic({0x206ad60?, 0x3dd8c30?})
12.     /opt/hostedtoolcache/go/1.22.2/x64/src/runtime/panic.go:770 +0x132
13. net/url.(*URL).Query(0x1000?)
14.     /opt/hostedtoolcache/go/1.22.2/x64/src/net/url/url.go:1122 +0xe
15. github.com/gin-gonic/gin.(*Context).initQueryCache(...)
16.     /home/runner/go/pkg/mod/github.com/gin-gonic/gin@v1.9.1/context.go:461
17. github.com/gin-gonic/gin.(*Context).GetQueryArray(0xc000126a00, {0x2392c3d, 0xf})
18.     /home/runner/go/pkg/mod/github.com/gin-gonic/gin@v1.9.1/context.go:471 +0x47
19. github.com/gin-gonic/gin.(*Context).GetQuery(...)
20.     /home/runner/go/pkg/mod/github.com/gin-gonic/gin@v1.9.1/context.go:445
21. github.com/gin-gonic/gin.(*Context).Query(0x2b5c688?, {0x2392c3d?, 0x2b6f428?})
22.     /home/runner/go/pkg/mod/github.com/gin-gonic/gin@v1.9.1/context.go:416 +0x18
23. github.com/HIDDEN_GH_ORG_NAME/api/internal/app/api/handlers/v1.OrganizationAPI.ListOrgsHandler({{0x2b5b880, 0xc0002b6150}, {{0x2b5c688, 0xc000cde910}, {0x2b6f428, 0xc000cde920}, {0x2b565b0, 0xc000cde930}, {0x2b666e0, 0xc000cde940}, ...}, ...}, ...)
24.     /home/runner/work/api/api/internal/app/api/handlers/v1/organizations.go:217 +0x2fe
25. github.com/HIDDEN_GH_ORG_NAME/api/internal/app/api/handlers/v1.Test_ListOrgs(0xc00101ab60)
26.     /home/runner/work/api/api/internal/app/api/handlers/v1/organizations_test.go:88 +0x95b
27. testing.tRunner(0xc00101ab60, 0x24f1320)
28.     /opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1689 +0xfb
29. created by testing.(*T).Run in goroutine 1
30.     /opt/hostedtoolcache/go/1.22.2/x64/src/testing/testing.go:1742 +0x390
31. FAIL    github.com/HIDDEN_GH_ORG_NAME/api/internal/app/api/handlers/v1  10.886s

Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.05%. Comparing base (3dc1cd6) to head (ff8ef98).
Report is 58 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3969      +/-   ##
==========================================
- Coverage   99.21%   99.05%   -0.16%     
==========================================
  Files          42       44       +2     
  Lines        3182     2751     -431     
==========================================
- Hits         3157     2725     -432     
+ Misses         17       15       -2     
- Partials        8       11       +3     
Flag Coverage Δ
?
-tags "sonic avx" 99.04% <100.00%> (?)
-tags go_json 99.04% <100.00%> (?)
-tags nomsgpack 99.03% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 99.05% <100.00%> (-0.16%) ⬇️
go-1.22 99.05% <100.00%> (?)
macos-latest 99.05% <100.00%> (-0.16%) ⬇️
ubuntu-latest 99.05% <100.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adrianosela
Copy link
Contributor Author

Hey @RedCrazyGhost - not sure how to get the codecov test to pass here? I tested all branches of the added code and existing code in this function?

@RedCrazyGhost
Copy link
Contributor

I checked the relevant information of codecov and found that someone had modified the configuration file of codecov two days ago, but this configuration file could not make the code review of codecov meet the normal expectations.

For previous reports on codecov code review not meeting expectations, please refer to: #3956

Now we mainly use manual code review, ignoring the failure of codecov

Copy link
Contributor

@RedCrazyGhost RedCrazyGhost left a comment

Choose a reason for hiding this comment

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

I have completed the code review, LGTM.

@appleboy please code review.

@appleboy appleboy changed the title Verify URL is Non-nil in initQueryCache() fix(context): verify URL is Non-nil in initQueryCache() May 19, 2024
@appleboy appleboy added this to the v1.11 milestone May 19, 2024
@appleboy appleboy merged commit e0d46de into gin-gonic:master May 19, 2024
24 of 25 checks passed
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

3 participants