Skip to content

Commit

Permalink
transaction: Add a default finalizer checking if transaction has ended
Browse files Browse the repository at this point in the history
Add a dynamic check on transaction during its finalization, ensuring
that end is called and their resources are released.

It's a programmer error not to do that, so we should panic in such case.

Testing of this code-path requires wrapping a bit panic() because we
we can't recover from a panic happening in a goroutine.
  • Loading branch information
3v1n0 committed Oct 12, 2023
1 parent d597fe6 commit 872161d
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 1 deletion.
36 changes: 35 additions & 1 deletion transaction.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import "C"
import (
"errors"
"fmt"
"runtime"
"runtime/cgo"
"strings"
"sync/atomic"
Expand Down Expand Up @@ -253,7 +254,7 @@ func Start(service, user string, handler ConversationHandler) (*Transaction, err
// StartFunc registers the handler func as a conversation handler and starts
// the transaction (see Start() documentation).
func StartFunc(service, user string, handler func(Style, string) (string, error)) (*Transaction, error) {
return Start(service, user, ConversationFunc(handler))
return start(service, user, ConversationFunc(handler), "")
}

// StartConfDir initiates a new PAM transaction. Service is treated identically to
Expand Down Expand Up @@ -281,6 +282,12 @@ func StartConfDir(service, user string, handler ConversationHandler, confDir str
return start(service, user, handler, confDir)
}

type panicHandlerFunc func(any)

var defaultPanicHandler = func(v any) { panic(v) }

var panicHandler panicHandlerFunc = defaultPanicHandler

func start(service, user string, handler ConversationHandler, confDir string) (*Transaction, error) {
switch handler.(type) {
case BinaryConversationHandler:
Expand All @@ -295,6 +302,33 @@ func start(service, user string, handler ConversationHandler, confDir string) (*
conv: &C.struct_pam_conv{},
c: cgo.NewHandle(handler),
}

callers := make([]uintptr, 10)
haveCallerInfo := runtime.Callers(2, callers) > 0
runtime.SetFinalizer(t, func(t *Transaction) {
handle := atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&t.handle)))
if handle == nil {
return
}
if haveCallerInfo {
stackTrace := make([]string, 0, len(callers))
for i := 0; i < len(callers); i += 1 {
frame, more := runtime.CallersFrames(callers).Next()
stackTrace = append(stackTrace, fmt.Sprintf("%s:%d",
frame.File, frame.Line))
if !more {
break
}
callers = callers[1:]
}
panicHandler(fmt.Sprintf("Transaction %p was never ended. "+
"Initialization was at:\n %s",
t, strings.Join(stackTrace, "\n ")))
} else {
panicHandler(fmt.Sprintf("Transaction %p was never ended", t))
}
})

C.init_pam_conv(t.conv, C.uintptr_t(t.c))
s := C.CString(service)
defer C.free(unsafe.Pointer(s))
Expand Down
69 changes: 69 additions & 0 deletions transaction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import (
"os"
"os/user"
"path/filepath"
"regexp"
"runtime"
"testing"
"time"
)

func maybeEndTransaction(t *testing.T, tx *Transaction) {
Expand Down Expand Up @@ -550,6 +553,72 @@ func Test_Status(t *testing.T) {
}
}

func Test_Finalizer(t *testing.T) {
if !CheckPamHasStartConfdir() {
t.Skip("this requires PAM with Conf dir support")
}

func() {
tx, err := StartConfDir("permit-service", "", nil, "test-services")
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
}()

runtime.GC()
// sleep to switch to finalizer goroutine
time.Sleep(5 * time.Millisecond)
}

func Test_FinalizerNotCleanedUp(t *testing.T) {
if !CheckPamHasStartConfdir() {
t.Skip("this requires PAM with Conf dir support")
}

panicChan := make(chan any)
panicHandler = func(v any) { panicChan <- v }
go func() { time.Sleep(time.Second * 2); panicChan <- errors.New("<timeout>") }()

func() {
_, err := StartConfDir("permit-service", "", nil, "test-services")
if err != nil {
t.Fatalf("start #error: %v", err)
}
}()

runtime.GC()

panicMsg := (<-panicChan).(string)
panicHandler = defaultPanicHandler

match, err := regexp.MatchString(
"Transaction 0x[0-9a-f]+ was never ended", panicMsg)
if err != nil {
t.Fatalf("unexpected error %v", err)
}
if !match {
t.Fatalf("no match in result:\n%s", panicMsg)
}
match, err = regexp.MatchString(
"transaction.go:[0-9]+", panicMsg)
if err != nil {
t.Fatalf("unexpected error %v", err)
}
if !match {
t.Fatalf("no match in result:\n%s", panicMsg)
}
match, err = regexp.MatchString(
"transaction_test.go:[0-9]+", panicMsg)
if err != nil {
t.Fatalf("unexpected error %v", err)
}
if !match {
t.Fatalf("no match in result:\n%s", panicMsg)
}
fmt.Println("I'd be done...")
}

func TestFailure_001(t *testing.T) {
tx := Transaction{}
_, err := tx.GetEnvList()
Expand Down

0 comments on commit 872161d

Please sign in to comment.