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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悶 Param string data buffer is reused, causing unexpected side effects #426

Closed
ViRb3 opened this issue May 29, 2020 · 9 comments
Closed

Comments

@ViRb3
Copy link

@ViRb3 ViRb3 commented May 29, 2020

Fiber version/commit

v1.10.1

Issue description

The method fiber.Ctx.Params(key string) string returns strings that share the same data buffer. This means that once a new string is returned, all previously returned strings will be changed to the new string's content, no matter how much they have been copied and passed around, since their data buffer is the same. This behavior is clearly seen in PoC below. unsafe is used to parse the header of each string returned from the Params method and then its content is printed. Notice how the data pointer is identical for all strings, meaning that they all use the same data buffer.

The only workaround right now is to explicitly copy the data buffer into a new buffer and create a string from the new buffer:

newBuffer := make([]byte, len(result))
copy(newBuffer, result)
newResult := string(newBuffer)

This behavior is absolutely unexpected, as strings in Go are expected to be immutable. I have not tracked down exactly what causes this issue, but I suspect that it may be a side effect from an inherited optimization by fasthttp. If this is true, I suspect that there are other methods that behave the same way as well. This behavior should be changed, or a clear documentation notice should be placed that indicates this side effect.

PoC

package main

import (
	"fmt"
	"github.com/gofiber/fiber"
	"net/http"
	"reflect"
	"sync"
	"unsafe"
)

var results []string
var wg sync.WaitGroup

func main() {
	go server()
	client()
	wg.Wait()
}

func client() {
	wg.Add(3)
	http.Get("http://127.0.0.1:3000/aaa")
	http.Get("http://127.0.0.1:3000/bbbb")
	http.Get("http://127.0.0.1:3000/c")
}

func server() {
	app := fiber.New()
	app.Get("/:test", func(c *fiber.Ctx) {
		result := c.Params("test")
		hdr := (*reflect.StringHeader)(unsafe.Pointer(&result))
		results = append(results, result)
		fmt.Printf("Param string: %+v Value:%s\n", hdr, result)
		fmt.Printf("Results: %+v\n", results)
		wg.Done()
	})
	app.Listen(3000)
}

Output

Param string: &{Data:824633868529 Len:3} Value:aaa
Results: [aaa]
Param string: &{Data:824633868529 Len:4} Value:bbbb
Results: [bbb bbbb]
Param string: &{Data:824633868529 Len:1} Value:c
Results: [cbb cbbb c]
@welcome
Copy link

@welcome welcome bot commented May 29, 2020

Thanks for opening your first issue here! 馃帀 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@Fenny
Copy link
Member

@Fenny Fenny commented May 30, 2020

We use the same method as Fasthttp to convert strings and bytes without memory allocation, but requires you to never change the original byte array, but that's a reasonable contract in many cases. To persist values after returning from the handler, you can use the build-in Immutable setting in Fiber.

Reference: #185

app := fiber.New(&fiber.Settings{
    Immutable: true,
})

However, we could make an exception to allocate parameters when c.Params() is called and keep 0 allocations on matching / hot paths. @ReneWerner87 thoughts?

@ViRb3
Copy link
Author

@ViRb3 ViRb3 commented May 30, 2020

Oh, I was not aware of the immutable setting. I wouldn't mind keeping the current behavior, but there should be clear documentation that states such side effects - perhaps a general warning in the start of the project, so people are aware that this is even a possible side effect, and maybe note down the scope of returned variables on each method's documentation, like fasthttp does it. It took me a full day to track down this issue just because I just didn't expect it to be coming from fiber.

@Fenny
Copy link
Member

@Fenny Fenny commented May 30, 2020

You are right regarding the documentation, we could add some sort of warning/disclaimer regarding this issue and explain how it would be solvable using the Immutable setting or allocate/copy specific variables by using the default string or []byte types.

If you have any suggestions on how we could phrase/summary this behavior, I will add it to all README's and API Docs right away.

Suggestion:

Returned value is valid until the handler returns, do not store references, make copies instead or use the Immutable setting to do this for you. Keep in mind that this may result in memory allocation+copy.

@ViRb3
Copy link
Author

@ViRb3 ViRb3 commented May 30, 2020

My recommendation would be to make two changes.

First, add a dedicated section in the beginning of the docs that outlines this concept as a whole. I created a PR for that, mentioned above. Any feedback is very welcome.

Second, for each method that returns a non-immutable value, like the Params method discussed in this issue, add a warning to its docs, like the one you proposed. I allowed myself to re-phrase it to make it a bit shorter:

Returned value is only valid within the handler. Do not store any references. Make copies or use the Immutable setting instead.

@Fenny
Copy link
Member

@Fenny Fenny commented May 30, 2020

Awesome work @ViRb3, I merged your PR in the docs. I will add your short phrase to each non-immutable method this weekend.

Feel free to join us on Discord to have a chat!

@Fenny Fenny closed this as completed May 30, 2020
@gabzim
Copy link

@gabzim gabzim commented Jul 10, 2021

@Fenny when you say not to keep references to it, is it ok to pass it to the service layer if the handler will wait for the service to return before it returns? example:

app.Get("/users/:id", func(c *fiber.Ctx) {
		userID := c.Params("id")
		// is this ok?
		user, err := userService.FindByID(userID)
                // ... rest of the code
	})

Assuming above that userID is only used to make a query to the DB and not stored anywhere in state beyond that use.

@ReneWerner87
Copy link
Member

@ReneWerner87 ReneWerner87 commented Jul 10, 2021

Yeah should be okay, the string is there until the response is written.

@calbot
Copy link

@calbot calbot commented Aug 14, 2021

I just got hit by this issue. I would like to suggest, (if it works) to return a new type something like below unsafestring.
This would make it explicit that the mutability MUST be considered when using the string. Go strings are always spoken of as immutable so it should probably be a new type. This way you could pass your unsafestring into functions and it would be clear that it requires special consideration. If you have fairy complicated program you could easily lose track of which strings are allowed to outlive the caller's scope.

Params, Query would return unsafestring instead of string

//Use the Copy method if you need it outside the scope
//of the fiber handler. It's ok to use UNSAFE to avoid a copy 
//if you know that that the result won't outlive the fiber handler

type unsafestring string

func (u unsafestring) Copy() string {
	return fiberUtils.ImmutableString(string(u))
}

func (u unsafestring) UNSAFE() string {
	return string(u)
}

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

No branches or pull requests

5 participants