Fix context.Copy() race condition

[raphaelgavache]

This PR aims to point an existing race condition with the gin.Context and propose a solution.

The gin.Context stores keys in a map[string]interface{}. The function gin.Context.Copy() (context.go:79) is supposed to give a thread safe copy of the initial gin.Context.

Instead of making a proper copy of the context.Keys, it copies the references of the map. This creates a race condition whenever the map context.Keys is accessed by concurrent go-routines.

With the following script you will get the raise condition instantly. The fix in the PR will solve it.

package main

import (
        "github.com/gin-gonic/gin"
        "math/rand"
)

func main() {

        r := gin.Default()
        r.GET("/", func(ctx *gin.Context) {
                ctx.Set("1", 0)
                ctx.Set("2", 0)

                // Sending a copy of the gin.Context to two separate routines
                go ReadWriteKeys(ctx.Copy())
                go ReadWriteKeys(ctx.Copy())
        })
        r.Run(":8080")
}

func ReadWriteKeys(ctx *gin.Context) {
        for {
                ctx.Set("1", rand.Int())
                ctx.Set("2", ctx.Value("1"))
        }
}

[codecov]

Codecov Report

Merging #1020 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1020      +/-   ##
==========================================
+ Coverage   98.49%   98.49%   +<.01%     
==========================================
  Files          41       41              
  Lines        2059     2062       +3     
==========================================
+ Hits         2028     2031       +3     
  Misses         19       19              
  Partials       12       12

Impacted Files	Coverage Δ
context.go	98.34% <100%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62749f0...05fa248. Read the comment docs.

[javierprovecho]

@furmmon can you add a test at context_test.go, similar to your example code, or just checking that the reference of the map from the copied context is different from the original?

[raphaelgavache]

Sure, couldn't find a simple test for that at first. This test fails on the current master

[gbbr]

@javierprovecho this seems like a fairly important thing to fix in a trivial set of changes - any plans on getting it in?

[raphaelgavache]

hey @appleboy, this is still an existing problem with the gin-gonic library, could you take a look at it
if you test the script above and curl localhost:8080 you will see instantly the race condition

[thinkerou]

Hi @furmmon you can add the following test code:

diff --git a/githubapi_test.go b/githubapi_test.go
index 6b56a2b..d9a1629 100644
--- a/githubapi_test.go
+++ b/githubapi_test.go
@@ -306,6 +306,29 @@ func TestShouldBindUri(t *testing.T) {
        assert.Equal(t, "ShouldBindUri test OK", w.Body.String())
 }

+func TestRaceContextCopy(t *testing.T) {
+       DefaultWriter = os.Stdout
+       router := Default()
+       router.GET("/test/copy/race", func(c *Context) {
+               c.Set("1", 0)
+               c.Set("2", 0)
+
+               // Sending a copy of the Context to two separate routines
+               go readWriteKeys(c.Copy())
+               go readWriteKeys(c.Copy())
+               c.String(http.StatusOK, "run OK, no panics")
+       })
+       w := performRequest(router, "GET", "/test/copy/race")
+       assert.Equal(t, "run OK, no panics", w.Body.String())
+}
+
+func readWriteKeys(c *Context) {
+       for {
+               c.Set("1", rand.Int())
+               c.Set("2", c.Value("1"))
+       }
+}
+
 func githubConfigRouter(router *Engine) {
        for _, route := range githubAPI {
                router.Handle(route.method, route.path, func(c *Context) {
(END)

If use old gin, the case will panics, I don't know how to catch the panics using assert.Panics?

[thinkerou]

@appleboy please review the pr, thanks!