From d3666c9152583953b447275a9583301d405275d9 Mon Sep 17 00:00:00 2001
From: Laurence Withers <l@lwithers.me.uk>
Date: Sun, 18 Feb 2024 14:14:31 +0000
Subject: [PATCH] Fix slice gotchas including concurrency race

If the []Attr slice passed to Conn.Entry was used in several concurrent
calls, and it had sufficient capacity that append()ing to it succeeded
without reallocating the slice, then a data race would exist.

Fix this, and fix the efficiency gotcha which expected the attribute
slice to have additional capacity, by instead always constructing a new
final slice of []Attr and using a sync.Pool to avoid reallocating them.

Fixes go test -race, and improves the benchmark. Before:
	goos: linux
	goarch: amd64
	pkg: src.lwithers.me.uk/go/journal
	cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
	BenchmarkEntry-8   	8345163	    2139 ns/op	   848 B/op	    15 allocs/op
	PASS
	ok  	src.lwithers.me.uk/go/journal	20.034s

After:
	goos: linux
	goarch: amd64
	pkg: src.lwithers.me.uk/go/journal
	cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
	BenchmarkEntry-8   	12611158	    1445 ns/op	   221 B/op	     7 allocs/op
	PASS
	ok  	src.lwithers.me.uk/go/journal	19.673s
---
 conn.go | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/conn.go b/conn.go
index 4ab8b2c..e94ff5a 100644
--- a/conn.go
+++ b/conn.go
@@ -2,6 +2,7 @@ package journal
 
 import (
 	"net"
+	"slices"
 	"sync"
 )
 
@@ -23,6 +24,7 @@ type Conn struct {
 
 	s    *net.UnixConn
 	bufs sync.Pool
+	atts sync.Pool
 }
 
 // Connect to the systemd journal. If the path string is empty, then it uses the
@@ -40,23 +42,26 @@ func Connect(path string) (*Conn, error) {
 		return nil, err
 	}
 
-	return &Conn{
+	c := &Conn{
 		s: s,
 		bufs: sync.Pool{
 			New: func() any {
 				return &net.Buffers{}
 			},
 		},
-	}, nil
+	}
+	c.atts = sync.Pool{
+		New: func() any {
+			return make([]Attr, 0, 2+len(c.Common))
+		},
+	}
+	return c, nil
 }
 
 var messageAttrKey = AttrKey{key: "MESSAGE"}
 
 // Entry emits a log entry. It will add PRIORITY and MESSAGE key/value pairs
 // as well as any Common attributes.
-//
-// Note: to avoid allocation / garbage, ensure attrs has capacity for an extra
-// 2+len(c.Common) values.
 func (c *Conn) Entry(pri Priority, msg string, attrs []Attr) {
 	err := c.EntryErr(pri, msg, attrs)
 	switch {
@@ -72,12 +77,18 @@ func (c *Conn) Entry(pri Priority, msg string, attrs []Attr) {
 // EntryErr is like Entry, but will propagate errors to the caller, rather than
 // using the built-in error handler.
 func (c *Conn) EntryErr(pri Priority, msg string, attrs []Attr) error {
-	attrs = append(attrs, pri.Attr(), Attr{
+	a := c.atts.Get().([]Attr)
+	a = slices.Grow(a[:0], 2+len(c.Common)+len(attrs))
+	a = append(a, pri.Attr(), Attr{
 		Key:   messageAttrKey,
 		Value: []byte(msg),
 	})
-	attrs = append(attrs, c.Common...)
-	return c.WriteAttrs(attrs)
+	a = append(a, c.Common...)
+	a = append(a, attrs...)
+	err := c.WriteAttrs(attrs)
+	slices.Delete(a, 0, len(a)) // ensure GC doesn't see refs to old data
+	c.atts.Put(a)
+	return err
 }
 
 // WriteAttrs is a low-level method which writes a journal entry comprised of
@@ -86,6 +97,7 @@ func (c *Conn) WriteAttrs(attrs []Attr) error {
 	buf := c.bufs.Get().(*net.Buffers)
 	*buf = (*buf)[:0]
 	err := WireWrite(buf, c.s, attrs)
+	slices.Delete(*buf, 0, len(*buf)) // ensure GC doesn't see refs to old data
 	c.bufs.Put(buf)
 	return err
 }