gzip icon indicating copy to clipboard operation
gzip copied to clipboard

Is there a race condition

Open sdemjanenko opened this issue 1 year ago • 1 comments

I was looking at the func (g *gzipHandler) Handle(c *gin.Context) method and it appears to me that there could be a case where gz := g.gzPool.Get().(*gzip.Writer) is used after being returned to sync.Pool.

Specifically in this code

gz := g.gzPool.Get().(*gzip.Writer)
defer g.gzPool.Put(gz)
defer gz.Reset(io.Discard)
gz.Reset(c.Writer)

c.Header("Content-Encoding", "gzip")
c.Header("Vary", "Accept-Encoding")
c.Writer = &gzipWriter{c.Writer, gz}
defer func() {
	gz.Close()
	c.Header("Content-Length", fmt.Sprint(c.Writer.Size()))
}()
c.Next()

It seems like it would be safer if the code was something like

gz := g.gzPool.Get().(*gzip.Writer)
defer g.gzPool.Put(gz)

// NOTE: There may be a better way than creating a func here
// My goal is to run gz.Reset(io.Discard) before g.gzPool.Put(gz)
// I also want gz.Close() to run before g.gzPool.Put(gz)
// I am not sure if gz.Close() should run after gz.Reset(io.Discard)
// I believe it currently runs after.
func() {
	defer gz.Reset(io.Discard)
	gz.Reset(c.Writer)

	c.Header("Content-Encoding", "gzip")
	c.Header("Vary", "Accept-Encoding")
	c.Writer = &gzipWriter{c.Writer, gz}
	defer func() {
		gz.Close()
		c.Header("Content-Length", fmt.Sprint(c.Writer.Size()))
	}()
        c.Next()
}()

It is possible that I am missing something that prevents a race condition like this from happening. I would love to learn more about this.

sdemjanenko avatar Sep 20 '24 23:09 sdemjanenko

I think i got the defer order wrong. Its a last-in-first-out stack. I think that means the order of the defer execution is

gz.Close()
c.Header("Content-Length", fmt.Sprint(c.Writer.Size()))

gz.Reset(io.Discard)

g.gzPool.Put(gz)

Which I interpret to be correct.

sdemjanenko avatar Sep 21 '24 00:09 sdemjanenko