fasthttp / router

Router implementation for fasthttp
BSD 3-Clause "New" or "Revised" License
463 stars 47 forks source link

ctx.userValues is shared across different request matches causing unpredictable results? #38

Closed gsych closed 4 years ago

gsych commented 4 years ago

Hi there! :)

I've prepared a short snippet program localizing the issue: go.mod:

module test

go 1.14

require (
    github.com/fasthttp/router v1.2.4
    github.com/valyala/fasthttp v1.15.1
)

main.go:

package main

import (
    httprouter "github.com/fasthttp/router"
    "github.com/valyala/fasthttp"
)

func main() {
    router := httprouter.New()

    var s string

    router.GET("/api/v1/path/special-path", func(ctx *fasthttp.RequestCtx) {
        println("from GET: " + s)
    })
    router.PATCH("/api/v1/path/{id}", func(ctx *fasthttp.RequestCtx) {
        s = ctx.UserValue("id").(string)
        println("from PATCH: " + s)
    })

    err := fasthttp.ListenAndServe("127.0.0.1:8080", router.Handler)
    if err != nil {
        panic(err)
    }
}

Then I make the following requests, one after another: curl --location --request PATCH 'http://127.0.0.1:8080/api/v1/path/aaabbbcccdddzzz' curl --location --request GET 'http://127.0.0.1:8080/api/v1/path/special-path'

The output I expect to see:

from PATCH: aaabbbcccdddzzz
from GET: aaabbbcccdddzzz

The actual output:

from PATCH: aaabbbcccdddzzz
from GET: special-pathzzz

As you can see the special-path path part overwrites part of the aaabbbcccdddzzz word.

gsych commented 4 years ago

Hi @savsgio !

Thank you for your reply. Yes, it was clear why this happens from the beginning. Still, I think there should be a note about that in the documentation or a comment to the UserValue func or anywhere else, otherwise in my opinion it's really misleading. I've spent several hours trying to figure out why the hell I have a data corruption and never had a second thought that I actually have to allocate a new copy. Having a proper allocation in the UserValue usage example would definitely prevent this.

Thank you for your effort!

savsgio commented 4 years ago

Hi @gsych,

Sorry for remove my answer, but i think the better solution it's save a copy directly to the user values and avoid confusions in a future.

I will upload the fix now.

savsgio commented 4 years ago

Fixed 😉

Thanks for your issue!

gsych commented 4 years ago

Superb!

sashayakovtseva commented 3 years ago

Hello @savsgio,

Looks like this issue still exists for regex params. Here no copy of path is made, so it is possible to have values corrupted.

sashayakovtseva commented 3 years ago

Added a PR with tests: #52. @savsgio Pls take a look.