Skip to content

Commit

Permalink
transaction: Add End() method and Remove Transaction finalizer
Browse files Browse the repository at this point in the history 
A PAM transaction needs to be ended in order to release the associated
resources, however this can't be sadly automated as the go finalizers
run in goroutines and this could cause problems to modules that we load.

In fact a module code may be called back during pam_end (to cleanup data
for example) and the module code could not be thread safe.

So let's make this more manual, but safer.
The transaction status is still preserved in the transaction so end will
be automatically called with the last-known status.

Closes: #14
  • Loading branch information
@3v1n0
3v1n0 committed Oct 12, 2023
1 parent b7f8e27 commit d597fe6
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 10 deletions.
36 changes: 28 additions & 8 deletions transaction.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import "C"
import (
"errors"
"fmt"
"runtime"
"runtime/cgo"
"strings"
"sync/atomic"
Expand Down Expand Up @@ -213,11 +212,17 @@ type Transaction struct {
c cgo.Handle
}

// transactionFinalizer cleans up the PAM handle and deletes the callback
// function.
func transactionFinalizer(t *Transaction) {
C.pam_end(t.handle, C.int(atomic.LoadInt32(&t.lastStatus)))
t.c.Delete()
// End cleans up the PAM handle and deletes the callback function.
// It must be called when done with the transaction.
func (t *Transaction) End() error {
handle := atomic.SwapPointer((*unsafe.Pointer)(unsafe.Pointer(&t.handle)), nil)
if handle == nil {
return nil
}

defer t.c.Delete()
return t.handlePamStatus(C.pam_end((*C.pam_handle_t)(handle),
C.int(atomic.LoadInt32(&t.lastStatus))))
}

// Allows to call pam functions managing return status
Expand All @@ -234,11 +239,19 @@ func (t *Transaction) handlePamStatus(cStatus C.int) error {
//
// All application calls to PAM begin with Start*. The returned
// transaction provides an interface to the remainder of the API.
//
// It's responsibility of the Transaction owner to release all the resources
// allocated underneath by PAM by calling End() once done.
//
// It's not advised to End the transaction using a runtime.SetFinalizer unless
// you're absolutely sure that your stack is multi-thread friendly (normally it
// is not!) and using a LockOSThread/UnlockOSThread pair.
func Start(service, user string, handler ConversationHandler) (*Transaction, error) {
return start(service, user, handler, "")
}

// StartFunc registers the handler func as a conversation handler.
// 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))
}
Expand All @@ -250,6 +263,13 @@ func StartFunc(service, user string, handler func(Style, string) (string, error)
//
// All application calls to PAM begin with Start*. The returned
// transaction provides an interface to the remainder of the API.
//
// It's responsibility of the Transaction owner to release all the resources
// allocated underneath by PAM by calling End() once done.
//
// It's not advised to End the transaction using a runtime.SetFinalizer unless
// you're absolutely sure that your stack is multi-thread friendly (normally it
// is not!) and using a LockOSThread/UnlockOSThread pair.
func StartConfDir(service, user string, handler ConversationHandler, confDir string) (*Transaction, error) {
if !CheckPamHasStartConfdir() {
return nil, &TransactionError{
Expand All @@ -276,7 +296,6 @@ func start(service, user string, handler ConversationHandler, confDir string) (*
c: cgo.NewHandle(handler),
}
C.init_pam_conv(t.conv, C.uintptr_t(t.c))
runtime.SetFinalizer(t, transactionFinalizer)
s := C.CString(service)
defer C.free(unsafe.Pointer(s))
var u *C.char
Expand All @@ -293,6 +312,7 @@ func start(service, user string, handler ConversationHandler, confDir string) (*
err = t.handlePamStatus(C.pam_start_confdir(s, u, t.conv, c, &t.handle))
}
if err != nil {
var _ = t.End()
return nil, err
}
return t, nil
Expand Down
47 changes: 45 additions & 2 deletions transaction_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,16 @@ import (
"testing"
)

func maybeEndTransaction(t *testing.T, tx *Transaction) {
if tx == nil {
return
}
err := tx.End()
if err != nil {
t.Fatalf("end #error: %v", err)
}
}

func TestPAM_001(t *testing.T) {
u, _ := user.Current()
if u.Uid != "0" {
Expand All @@ -18,6 +28,7 @@ func TestPAM_001(t *testing.T) {
tx, err := StartFunc("", "test", func(s Style, msg string) (string, error) {
return p, nil
})
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand Down Expand Up @@ -49,6 +60,7 @@ func TestPAM_002(t *testing.T) {
}
return "", errors.New("unexpected")
})
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand Down Expand Up @@ -83,6 +95,7 @@ func TestPAM_003(t *testing.T) {
Password: "secret",
}
tx, err := Start("", "", c)
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand All @@ -101,6 +114,7 @@ func TestPAM_004(t *testing.T) {
Password: "secret",
}
tx, err := Start("", "test", c)
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand All @@ -118,6 +132,7 @@ func TestPAM_005(t *testing.T) {
tx, err := StartFunc("passwd", "test", func(s Style, msg string) (string, error) {
return "secret", nil
})
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand All @@ -135,6 +150,7 @@ func TestPAM_006(t *testing.T) {
tx, err := StartFunc("passwd", u.Username, func(s Style, msg string) (string, error) {
return "secret", nil
})
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand All @@ -156,6 +172,7 @@ func TestPAM_007(t *testing.T) {
tx, err := StartFunc("", "test", func(s Style, msg string) (string, error) {
return "", errors.New("Sorry, it didn't work")
})
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand All @@ -179,6 +196,11 @@ func TestPAM_ConfDir(t *testing.T) {
Password: "wrongsecret",
}
tx, err := StartConfDir("permit-service", u.Username, c, "test-services")
defer func() {
if tx != nil {
tx.End()
}
}()
if !CheckPamHasStartConfdir() {
if err == nil {
t.Fatalf("start should have errored out as pam_start_confdir is not available: %v", err)
Expand All @@ -200,10 +222,13 @@ func TestPAM_ConfDir_FailNoServiceOrUnsupported(t *testing.T) {
c := Credentials{
Password: "secret",
}
_, err := StartConfDir("does-not-exists", u.Username, c, ".")
tx, err := StartConfDir("does-not-exists", u.Username, c, ".")
if err == nil {
t.Fatalf("authenticate #expected an error")
}
if tx != nil {
t.Fatalf("authenticate #unexpected transaction")
}
s := err.Error()
if len(s) == 0 {
t.Fatalf("error #expected an error message")
Expand All @@ -225,6 +250,7 @@ func TestPAM_ConfDir_InfoMessage(t *testing.T) {
}
return "", errors.New("unexpected")
}), "test-services")
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand All @@ -240,6 +266,7 @@ func TestPAM_ConfDir_InfoMessage(t *testing.T) {
func TestPAM_ConfDir_Deny(t *testing.T) {
u, _ := user.Current()
tx, err := StartConfDir("deny-service", u.Username, Credentials{}, "test-services")
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand All @@ -263,6 +290,7 @@ func TestPAM_ConfDir_PromptForUserName(t *testing.T) {
Password: "wrongsecret",
}
tx, err := StartConfDir("succeed-if-user-test", "", c, "test-services")
defer maybeEndTransaction(t, tx)
if !CheckPamHasStartConfdir() {
if err == nil {
t.Fatalf("start should have errored out as pam_start_confdir is not available: %v", err)
Expand All @@ -285,6 +313,7 @@ func TestPAM_ConfDir_WrongUserName(t *testing.T) {
Password: "wrongsecret",
}
tx, err := StartConfDir("succeed-if-user-test", "", c, "test-services")
defer maybeEndTransaction(t, tx)
if !CheckPamHasStartConfdir() {
if err == nil {
t.Fatalf("start should have errored out as pam_start_confdir is not available: %v", err)
Expand All @@ -306,9 +335,13 @@ func TestPAM_ConfDir_WrongUserName(t *testing.T) {
}

func TestItem(t *testing.T) {
tx, _ := StartFunc("passwd", "test", func(s Style, msg string) (string, error) {
tx, err := StartFunc("passwd", "test", func(s Style, msg string) (string, error) {
return "", nil
})
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}

s, err := tx.GetItem(Service)
if err != nil {
Expand Down Expand Up @@ -343,6 +376,7 @@ func TestEnv(t *testing.T) {
tx, err := StartFunc("", "", func(s Style, msg string) (string, error) {
return "", nil
})
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand Down Expand Up @@ -486,6 +520,7 @@ func Test_Status(t *testing.T) {
}

tx, err := StartConfDir(serviceName, "user", c, servicePath)
defer maybeEndTransaction(t, tx)
if err != nil {
t.Fatalf("start #error: %v", err)
}
Expand Down Expand Up @@ -586,3 +621,11 @@ func TestFailure_009(t *testing.T) {
t.Fatalf("getenvlist #expected an error")
}
}

func TestFailure_010(t *testing.T) {
tx := Transaction{}
err := tx.End()
if err != nil {
t.Fatalf("end #unexpected error %v", err)
}
}

0 comments on commit d597fe6

Please sign in to comment.