test(integration): improve valgrind

.github/workflows/make-integration-tests.yml

@@ -24,6 +24,10 @@ on:
         description: 'runner type'
         required: true
         type: string
+      use-valgrind:
+        required: true
+        default: false
+        type: boolean
 
 jobs:
   test-agent:
@@ -77,7 +81,7 @@ jobs:
         run: |
           docker exec \
           -e PHPS=${{matrix.php}} \
-          -e INTEGRATION_ARGS="-license ${{secrets.NR_TEST_LICENSE}} -collector ${{secrets.NR_COLLECTOR_HOST}} -agent agent/modules/newrelic.so" \
+          -e INTEGRATION_ARGS="-license ${{secrets.NR_TEST_LICENSE}} -collector ${{secrets.NR_COLLECTOR_HOST}} -agent agent/modules/newrelic.so ${{ inputs.use-valgrind && '-valgrind /usr/bin/valgrind' }}" \
           -e APP_supportability=${{secrets.APP_SUPPORTABILITY}} \
           -e ACCOUNT_supportability=${{secrets.ACCOUNT_SUPPORTABILITY}} \
           -e ACCOUNT_supportability_trusted=${{secrets.ACCOUNT_SUPPORTABILITY_TRUSTED}} \

.github/workflows/run-integration-tests.yml

@@ -26,7 +26,11 @@ on:
         options: 
         - ubuntu-22.04-arm
         - ubuntu-24.04-arm
-
+      use-valgrind:
+        description: 'Use valgrind?'
+        required: true
+        default: false
+        type: boolean
 jobs:
   build-test-runner-amd64:
     uses: ./.github/workflows/make-for-platform-on-arch.yml
@@ -78,6 +82,7 @@ jobs:
     with:
       origin: ${{ inputs.origin }}
       ref: ${{ inputs.ref }}
+      use-valgrind: ${{ inputs.use-valgrind }}
       arch: amd64
       runs-on: ubuntu-latest
       continue-on-error: false
@@ -88,6 +93,7 @@ jobs:
     with:
       origin: ${{ inputs.origin }}
       ref: ${{ inputs.ref }}
+      use-valgrind: ${{ inputs.use-valgrind }}
       arch: arm64
       runs-on: ${{inputs.runner-type}}
       continue-on-error: true

daemon/cmd/integration_runner/main.go

@@ -344,6 +344,10 @@ func main() {
 
 	ctx = integration.NewContext(*flagPHP, *flagCGI)
 	ctx.Valgrind = *flagValgrind
+	if ctx.Valgrind != "" && *flagWorkers != 1 {
+		fmt.Fprintf(os.Stderr, "cannot run valgrind on multiple workers\n")
+		os.Exit(1)
+	}
 	ctx.Timeout = *flagTimeout
 
 	// Settings common to all tests.

daemon/internal/newrelic/integration/transaction.go

@@ -48,7 +48,7 @@ func PhpTx(src Script, env, settings map[string]string, ctx *Context) (Tx, error
 
 	args := phpArgs(nil, filepath.Base(src.Name()), false, settings)
 
-	if ctx.Valgrind != "" {
+	if ctx.Valgrind != "" && settings["newrelic.appname"] != "skipif" {
 		txn = &ValgrindCLI{
 			CLI: CLI{
 				Path: ctx.PHP,
@@ -81,6 +81,7 @@ func PhpTx(src Script, env, settings map[string]string, ctx *Context) (Tx, error
 // See: https://tools.ietf.org/html/rfc3875
 func CgiTx(src Script, env, settings map[string]string, headers http.Header, ctx *Context) (Tx, error) {
 	var err error
+	var txn Tx
 
 	req := &http.Request{
 		Method:     env["REQUEST_METHOD"],
@@ -105,34 +106,69 @@ func CgiTx(src Script, env, settings map[string]string, headers http.Header, ctx
 		return nil, fmt.Errorf("unable to create cgi request: %v", err)
 	}
 
-	tx := &CGI{
-		request: req,
-		handler: &cgi.Handler{
-			Path: ctx.CGI,
-			Dir:  src.Dir(),
-			Args: phpArgs(nil, "", false, settings),
-		},
-	}
+	if ctx.Valgrind != "" {
+		tx := &ValgrindCGI{
+			CGI: CGI{
+				request: req,
+				handler: &cgi.Handler{
+					Path: ctx.CGI,
+					Dir:  src.Dir(),
+					Args: phpArgs(nil, "", false, settings),
+				},
+			},
+			Valgrind: ctx.Valgrind,
+			Timeout:  ctx.Timeout,
+		}
+		tx.handler.Env = append(tx.handler.Env,
+			"SCRIPT_FILENAME="+scriptFile,
+			"SCRIPT_NAME=/"+filepath.Base(src.Name()),
+			"REDIRECT_STATUS=200")
+		tx.handler.Env = append(tx.handler.Env, flatten(env)...)
+
+		// If the environment includes a REQUEST_URI and doesn't include an explicit
+		// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
+		// attempting to guess what the PATH_INFO should be.
+		if ruri, ok := env["REQUEST_URI"]; ok {
+			if _, ok := env["PATH_INFO"]; !ok {
+				tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
+			}
+		}
 
-	tx.handler.Env = append(tx.handler.Env,
-		"SCRIPT_FILENAME="+scriptFile,
-		"SCRIPT_NAME=/"+filepath.Base(src.Name()),
-		"REDIRECT_STATUS=200")
-	tx.handler.Env = append(tx.handler.Env, flatten(env)...)
-
-	// If the environment includes a REQUEST_URI and doesn't include an explicit
-	// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
-	// attempting to guess what the PATH_INFO should be.
-	if ruri, ok := env["REQUEST_URI"]; ok {
-		if _, ok := env["PATH_INFO"]; !ok {
-			tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
+		if !src.IsFile() {
+			return withTempFile(tx, src.Name(), src.Bytes()), nil
+		}
+		txn = tx
+	} else {
+		tx := &CGI{
+			request: req,
+			handler: &cgi.Handler{
+				Path: ctx.CGI,
+				Dir:  src.Dir(),
+				Args: phpArgs(nil, "", false, settings),
+			},
+		}
+		tx.handler.Env = append(tx.handler.Env,
+			"SCRIPT_FILENAME="+scriptFile,
+			"SCRIPT_NAME=/"+filepath.Base(src.Name()),
+			"REDIRECT_STATUS=200")
+		tx.handler.Env = append(tx.handler.Env, flatten(env)...)
+
+		// If the environment includes a REQUEST_URI and doesn't include an explicit
+		// PATH_INFO, we'll set it here to avoid problems with Go's CGI package
+		// attempting to guess what the PATH_INFO should be.
+		if ruri, ok := env["REQUEST_URI"]; ok {
+			if _, ok := env["PATH_INFO"]; !ok {
+				tx.handler.Env = append(tx.handler.Env, "PATH_INFO="+ruri)
+			}
 		}
-	}
 
-	if !src.IsFile() {
-		return withTempFile(tx, src.Name(), src.Bytes()), nil
+		if !src.IsFile() {
+			return withTempFile(tx, src.Name(), src.Bytes()), nil
+		}
+		txn = tx
 	}
-	return tx, nil
+
+	return txn, nil
 }
 
 // phpArgs builds the command line for a PHP process.

daemon/internal/newrelic/integration/valgrind.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
+	"net/http/httptest"
 	"sync"
 	"time"
 
@@ -24,6 +25,12 @@ type ValgrindCLI struct {
 	Timeout  time.Duration
 }
 
+type ValgrindCGI struct {
+	CGI
+	Valgrind string
+	Timeout  time.Duration
+}
+
 func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
 	if len(tx.Path) == 0 {
 		return nil, []byte("skip: executable not specified"), nil
@@ -34,8 +41,13 @@ func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
 	// valgrind reports to their tests in the same way we use the appname
 	// to link the transaction data. Failing that, perhaps we could use
 	// the valgrind process's pid instead.
-	valgrindMu.Lock()
-	defer valgrindMu.Unlock()
+	//
+	// We prevent concurrent invocations by disallowing valgrind when
+	// the threads argument is greater than 1. Previously, we used the
+	// commented out locks below. This was changed to allow subprocesses
+	// (i.e. curl) to run without deadlocking.
+	//valgrindMu.Lock()
+	//defer valgrindMu.Unlock()
 
 	cmd := valgrind.Memcheck(tx.Valgrind, "--quiet")
 	cmd.Args = append(cmd.Args, "--xml=yes")
@@ -105,6 +117,87 @@ func (tx *ValgrindCLI) Execute() (http.Header, []byte, error) {
 	return nil, output, err
 }
 
+func (tx *ValgrindCGI) Execute() (http.Header, []byte, error) {
+	if len(tx.handler.Path) == 0 {
+		return nil, []byte("skip: executable not specified"), nil
+	}
+
+	// For now, we don't have a mechanism to handle concurrent invocations
+	// of Valgrind. In the future, we could use the appname to connect
+	// valgrind reports to their tests in the same way we use the appname
+	// to link the transaction data. Failing that, perhaps we could use
+	// the valgrind process's pid instead.
+	//
+	// We prevent concurrent invocations by disallowing valgrind when
+	// the threads argument is greater than 1. Previously, we used the
+	// commented out locks below. This was changed to allow subprocesses
+	// (i.e. curl) to run without deadlocking.
+	//valgrindMu.Lock()
+	//defer valgrindMu.Unlock()
+
+	cmd := valgrind.Memcheck(tx.Valgrind, "--quiet")
+	cmd.Args = append(cmd.Args, "--xml=yes")
+	cmd.Args = append(cmd.Args, "--xml-socket="+valgrindLn.Addr().String())
+	cmd.Args = append(cmd.Args, "--")
+	cmd.Args = append(cmd.Args, tx.handler.Path)
+	if len(tx.handler.Args) > 0 {
+		cmd.Args = append(cmd.Args, tx.handler.Args...)
+	}
+
+	log.Debugf("command: %v", cmd)
+
+	// Replace the handler with valgrind
+	tx.handler.Path = cmd.Path
+	// The first Arg is the Path.
+	// ServeHTTP will re-add Path
+	// to the front of Args
+	tx.handler.Args = cmd.Args[1:]
+
+	ch := make(chan resultOrError, 1)
+	go func() {
+		var result resultOrError
+		result.R, result.E = acceptOneReport(tx.Timeout)
+		ch <- result
+	}()
+
+	resp := httptest.NewRecorder()
+	tx.handler.ServeHTTP(resp, tx.request)
+
+	vgOutput := <-ch
+
+	// Append the output from Valgrind to the test output.
+	//
+	// TODO: Eventually we want to report valgrind output separately, so we can
+	// treat valgrind errors similarly to failed test expectations.  i.e. We'd
+	// like to add them to Test.Failures. After all, each test has an implicit
+	// expectation that it will not exhibit memory bugs!
+	//
+	// TODO: Once the former is in place, we should be able to parse the memory
+	// leak reports produced by the Zend Memory Manager from the test output and
+	// report them the same way as valgrind errors.
+	output := resp.Body.Bytes()
+	if vgOutput.R != nil && len(vgOutput.R.Errors) > 0 {
+		// Safe to ignore the error here, Report.MarshalText() never fails.
+		data, _ := vgOutput.R.MarshalText()
+		output = append(output, '\n')
+		output = append(output, data...)
+	}
+
+	// Ensure a non-nil error is returned when valgrind detects errors.
+	// Otherwise, the test could be marked as passing if it does not have
+	// any expectations on the test output. This sucks.
+	//
+	// TODO: Remove this when valgrind errors can be treated as failed test
+	// expectations.
+	err := vgOutput.E
+	if err == nil && vgOutput.R != nil && len(vgOutput.R.Errors) > 0 {
+		err = fmt.Errorf("detected %d memory errors", len(vgOutput.R.Errors))
+	}
+
+	return resp.HeaderMap, output, err
+
+}
+
 // resultOrError is a poor man's sum type.
 type resultOrError struct {
 	R *valgrind.Report