Skip to content

test(uffd): lift cross-process test infrastructure for UFFD work #2461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ValentaTomas merged 2 commits into from
Apr 21, 2026
Merged

test(uffd): lift cross-process test infrastructure for UFFD work #2461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e416f5c
test(uffd): lift cross-process test infrastructure for UFFD work
ValentaTomas Apr 20, 2026
11a7aaa
test(uffd): align ported tests with FPR style
ValentaTomas Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 56 additions & 10 deletions packages/orchestrator/pkg/sandbox/uffd/userfaultfd/async_wp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ func TestAsyncWriteProtection(t *testing.T) {
operations []operation
expectedDirty []int
expectedClean []int
alwaysWP bool
}{
{
name: "4k read then write same page",
Expand Down Expand Up @@ -229,6 +230,59 @@ func TestAsyncWriteProtection(t *testing.T) {
expectedDirty: []int{0, 2},
expectedClean: []int{1, 3},
},

// alwaysWP tests: handler copies with UFFDIO_COPY_MODE_WP for all faults,
// including writes. WP_ASYNC must automatically clear the WP bit when the
// original access was a write. This validates the assumption that independent
// prefaulting (always copy with WP) works correctly.
{
name: "4k alwaysWP write to missing page",
pagesize: header.PageSize,
numberOfPages: 4,
alwaysWP: true,
operations: []operation{
{offset: 0, mode: operationModeWrite},
},
expectedDirty: []int{0},
},
{
name: "4k alwaysWP mixed writes and reads",
pagesize: header.PageSize,
numberOfPages: 4,
alwaysWP: true,
operations: []operation{
{offset: 0 * header.PageSize, mode: operationModeWrite},
{offset: 1 * header.PageSize, mode: operationModeRead},
{offset: 2 * header.PageSize, mode: operationModeWrite},
{offset: 3 * header.PageSize, mode: operationModeRead},
},
expectedDirty: []int{0, 2},
expectedClean: []int{1, 3},
},
{
name: "hugepage alwaysWP write to missing page",
pagesize: header.HugepageSize,
numberOfPages: 4,
alwaysWP: true,
operations: []operation{
{offset: 0, mode: operationModeWrite},
},
expectedDirty: []int{0},
},
{
name: "hugepage alwaysWP mixed writes and reads",
pagesize: header.HugepageSize,
numberOfPages: 4,
alwaysWP: true,
operations: []operation{
{offset: 0 * header.HugepageSize, mode: operationModeWrite},
{offset: 1 * header.HugepageSize, mode: operationModeRead},
{offset: 2 * header.HugepageSize, mode: operationModeWrite},
{offset: 3 * header.HugepageSize, mode: operationModeRead},
},
expectedDirty: []int{0, 2},
expectedClean: []int{1, 3},
},
}

for _, tt := range tests {
Expand All @@ -238,19 +292,11 @@ func TestAsyncWriteProtection(t *testing.T) {
h, err := configureCrossProcessTest(t, testConfig{
pagesize: tt.pagesize,
numberOfPages: tt.numberOfPages,
alwaysWP: tt.alwaysWP,
})
require.NoError(t, err)

for i, op := range tt.operations {
switch op.mode {
case operationModeRead:
err := h.executeRead(t.Context(), op)
require.NoError(t, err, "step %d: read at offset %d", i, op.offset)
case operationModeWrite:
err := h.executeWrite(t.Context(), op)
require.NoError(t, err, "step %d: write at offset %d", i, op.offset)
}
}
h.executeAll(t, tt.operations)

pagemap, err := testutils.NewPagemapReader()
require.NoError(t, err)
Expand Down
9 changes: 8 additions & 1 deletion packages/orchestrator/pkg/sandbox/uffd/userfaultfd/cross_process_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,13 @@ func configureCrossProcessTest(t *testing.T, tt testConfig) (*testHandler, error
err = register(uffdFd, memoryStart, uint64(size), UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP)
require.NoError(t, err)

cmd := exec.CommandContext(t.Context(), os.Args[0], "-test.run=TestHelperServingProcess")
cmd := exec.CommandContext(t.Context(), os.Args[0], "-test.run=TestHelperServingProcess", "-test.timeout=0")
cmd.Env = append(os.Environ(), "GO_TEST_HELPER_PROCESS=1")
cmd.Env = append(cmd.Env, fmt.Sprintf("GO_MMAP_START=%d", memoryStart))
cmd.Env = append(cmd.Env, fmt.Sprintf("GO_MMAP_PAGE_SIZE=%d", tt.pagesize))
if tt.alwaysWP {
Comment on lines 106 to +109

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Isolate GO_ALWAYS_WP from parent environment

The helper process environment is seeded from os.Environ() and GO_ALWAYS_WP is only appended when tt.alwaysWP is true, so any ambient GO_ALWAYS_WP value from the parent shell can override the test config and change helper behavior unexpectedly. This makes configureCrossProcessTest non-deterministic across environments (e.g., non-alwaysWP cases can still run with forced WP), which can produce flaky or misleading test results.

Useful? React with 👍 / 👎.

cmd.Env = append(cmd.Env, "GO_ALWAYS_WP=1")
}

dup, err := syscall.Dup(int(uffdFd))
require.NoError(t, err)
Comment on lines 108 to 114

@claude claude Bot Apr 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 In configureCrossProcessTest, the child environment is built from os.Environ() and GO_ALWAYS_WP=1 is only appended when tt.alwaysWP is true — it is never explicitly removed or overridden when false. If a developer runs GO_ALWAYS_WP=1 go test ./..., every spawned helper process inherits the variable via os.Environ(), causing non-alwaysWP subtests to run the alwaysWP code path and breaking test isolation. Fix by always explicitly setting GO_ALWAYS_WP to either "0" or "1" rather than conditionally appending.

Extended reasoning...

What the bug is and how it manifests

In configureCrossProcessTest (cross_process_helpers_test.go:112-118), the child process command environment is assembled as:

cmd.Env = append(os.Environ(), "GO_TEST_HELPER_PROCESS=1")
// ...
if tt.alwaysWP {
    cmd.Env = append(cmd.Env, "GO_ALWAYS_WP=1")
}

The code only appends GO_ALWAYS_WP=1 when alwaysWP is true, but never removes or overrides it when alwaysWP is false. Because the base environment comes from os.Environ(), any value of GO_ALWAYS_WP present in the parent process is inherited unconditionally by every child.

The specific code path that triggers it

If a developer or CI runner executes GO_ALWAYS_WP=1 go test ./... (e.g., to quickly re-run only the alwaysWP subtests and then forgets to unset the variable), the parent process has GO_ALWAYS_WP=1 in its environment. os.Environ() captures that value, and every helper child launched — including those for alwaysWP=false subtests — inherits it. Inside crossProcessServe() (cross_process_helpers_test.go:303-305):

if os.Getenv("GO_ALWAYS_WP") == "1" {
    uffd.defaultCopyMode = UFFDIO_COPY_MODE_WP
}

This sets defaultCopyMode = UFFDIO_COPY_MODE_WP even for tests that are supposed to exercise the normal (non-WP) copy path.

Why existing code doesn't prevent it

The pattern cmd.Env = append(os.Environ(), ...) is standard Go helper-process idiom and is not inherently wrong — the issue is specifically that a conditionally appended variable is never explicitly cleared. Since cmd.Env is evaluated left-to-right and multiple occurrences of the same variable are resolved by position (first or last, depending on OS), and since the code never appends GO_ALWAYS_WP=0 for the false branch, there is no defensive override.

What the impact would be

Non-alwaysWP tests (e.g., "4k write to missing page") would run the helper with defaultCopyMode = UFFDIO_COPY_MODE_WP, meaning every UFFDIO_COPY carries the WP bit. The tests still pass because UFFD_FEATURE_WP_ASYNC automatically clears the WP bit on write faults, but the code path being exercised is the alwaysWP path, not the normal path. This defeats the purpose of having both variants and means regressions in the normal path would not be caught.

Step-by-step proof

  1. Developer runs: GO_ALWAYS_WP=1 go test -run TestAsyncWriteProtection ./pkg/sandbox/uffd/userfaultfd/
  2. The test binary's process environment contains GO_ALWAYS_WP=1.
  3. configureCrossProcessTest is called for the subtest "4k write to missing page" where tt.alwaysWP = false.
  4. cmd.Env = append(os.Environ(), ...) — GO_ALWAYS_WP=1 is now in cmd.Env.
  5. The if tt.alwaysWP branch is false, so nothing overrides it.
  6. The child starts and crossProcessServe reads os.Getenv("GO_ALWAYS_WP") == "1" → true → sets uffd.defaultCopyMode = UFFDIO_COPY_MODE_WP.
  7. The subtest runs the non-alwaysWP code path with WP always set — silent wrong behavior.

Addressing the refutation

The refuter correctly notes that GO_ALWAYS_WP is a brand-new, project-specific variable introduced by this very PR and would not appear in any real CI or external environment. This makes the scenario unlikely in practice. The tests also still pass due to WP_ASYNC. However, the issue is a straightforward hygiene fix (add an else branch setting GO_ALWAYS_WP=0, or filter it from os.Environ()) and the test isolation concern is legitimate — the purpose of the non-alwaysWP subtests is precisely to verify the non-WP code path. Silently running the wrong variant reduces the diagnostic value of the test suite.

Expand Down Expand Up @@ -293,6 +296,10 @@ func crossProcessServe() error {
return fmt.Errorf("exit creating uffd: %w", err)
}

if os.Getenv("GO_ALWAYS_WP") == "1" {
uffd.defaultCopyMode = UFFDIO_COPY_MODE_WP
}

offsetsFile := os.NewFile(uintptr(5), "offsets")

offsetsSignal := make(chan os.Signal, 1)
Expand Down
72 changes: 72 additions & 0 deletions packages/orchestrator/pkg/sandbox/uffd/userfaultfd/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@ import (
"context"
"fmt"
"sync"
"testing"
"unsafe"

"github.com/RoaringBitmap/roaring/v2"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/e2b-dev/infra/packages/orchestrator/pkg/sandbox/uffd/testutils"
)
Expand All @@ -19,6 +23,8 @@ type testConfig struct {
numberOfPages uint64
// Operations to trigger on the memory area.
operations []operation
// alwaysWP makes the handler copy with UFFDIO_COPY_MODE_WP for all faults.
alwaysWP bool
}

type operationMode uint32
Expand All @@ -44,6 +50,72 @@ type testHandler struct {
mutex sync.Mutex
}

func (h *testHandler) executeAll(t *testing.T, operations []operation) {
t.Helper()

for i, op := range operations {
err := h.executeOperation(t.Context(), op)
require.NoError(t, err, "step %d: %v at offset %d", i, op.mode, op.offset)
}
}

type pageExpectation uint8

const (
expectClean pageExpectation = iota // read-only: present + WP set
expectDirty // written: present + WP cleared
)

func (h *testHandler) checkDirtiness(t *testing.T, operations []operation) {
t.Helper()

pagemap, err := testutils.NewPagemapReader()
require.NoError(t, err)
defer pagemap.Close()

memStart := uintptr(unsafe.Pointer(&(*h.memoryArea)[0]))

// Track the final expected state per offset by replaying operations in order.
expected := make(map[uint]pageExpectation)

for _, op := range operations {
off := uint(op.offset)
switch op.mode {
case operationModeRead:
if _, seen := expected[off]; !seen {
expected[off] = expectClean
}
case operationModeWrite:
expected[off] = expectDirty
}
}

for off, expect := range expected {
entry, err := pagemap.ReadEntry(memStart + uintptr(off))
require.NoError(t, err, "pagemap read at offset %d", off)

switch expect {
case expectDirty:
assert.True(t, entry.IsPresent(), "written page at offset %d should be present", off)
assert.False(t, entry.IsWriteProtected(), "written page at offset %d should be dirty", off)
case expectClean:
assert.True(t, entry.IsPresent(), "read-only page at offset %d should be present", off)
assert.True(t, entry.IsWriteProtected(), "read-only page at offset %d should be clean", off)
}
}
}

func (h *testHandler) executeOperation(ctx context.Context, op operation) error {
switch op.mode {
case operationModeRead:
return h.executeRead(ctx, op)
case operationModeWrite:
return h.executeWrite(ctx, op)
default:
return fmt.Errorf("invalid operation mode: %d", op.mode)
}
}

func (h *testHandler) executeRead(ctx context.Context, op operation) error {
readBytes := (*h.memoryArea)[op.offset : op.offset+int64(h.pagesize)]

Expand Down
11 changes: 4 additions & 7 deletions packages/orchestrator/pkg/sandbox/uffd/userfaultfd/missing_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,27 +124,24 @@ func TestMissing(t *testing.T) {
h, err := configureCrossProcessTest(t, tt)
require.NoError(t, err)

for _, operation := range tt.operations {
if operation.mode == operationModeRead {
err := h.executeRead(t.Context(), operation)
require.NoError(t, err, "for operation %+v", operation)
}
}
h.executeAll(t, tt.operations)

expectedAccessedOffsets := getOperationsOffsets(tt.operations, operationModeRead|operationModeWrite)

accessedOffsets, err := h.offsetsOnce()
require.NoError(t, err)

assert.Equal(t, expectedAccessedOffsets, accessedOffsets, "checking which pages were faulted")

h.checkDirtiness(t, tt.operations)
})
}
}

func TestParallelMissing(t *testing.T) {
t.Parallel()

parallelOperations := 1_000_000
parallelOperations := 10_000

tt := testConfig{
pagesize: header.PageSize,
Expand Down
16 changes: 4 additions & 12 deletions packages/orchestrator/pkg/sandbox/uffd/userfaultfd/missing_write_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,32 +119,24 @@ func TestMissingWrite(t *testing.T) {
h, err := configureCrossProcessTest(t, tt)
require.NoError(t, err)

for _, operation := range tt.operations {
if operation.mode == operationModeRead {
err := h.executeRead(t.Context(), operation)
require.NoError(t, err, "for operation %+v", operation)
}

if operation.mode == operationModeWrite {
err := h.executeWrite(t.Context(), operation)
require.NoError(t, err, "for operation %+v", operation)
}
}
h.executeAll(t, tt.operations)

expectedAccessedOffsets := getOperationsOffsets(tt.operations, operationModeRead|operationModeWrite)

Comment on lines 119 to 125

@claude claude Bot Apr 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 The four tests in missing_write_test.go (TestMissingWrite, TestParallelMissingWrite, TestParallelMissingWriteWithPrefault, TestSerialMissingWrite) are missing the if os.Geteuid() \!= 0 { t.Skip(...) } guard present in missing_test.go (line 17) and async_wp_test.go (line 33). Without this guard, running these tests as a non-root user produces a hard kernel permission-denied failure via require.NoError rather than a clean skip, making the test suite appear broken to contributors. This is a pre-existing inconsistency that the PR refactors but does not fix.

Extended reasoning...

What the bug is and how it manifests

missing_write_test.go does not import the os package and contains no os.Geteuid() call anywhere in the file. All four of its top-level test functions — TestMissingWrite, TestParallelMissingWrite, TestParallelMissingWriteWithPrefault, and TestSerialMissingWrite — proceed directly to configureCrossProcessTest(t, tt) without first checking whether the calling process has root privileges.

The specific code path that triggers it

configureCrossProcessTest (cross_process_helpers_test.go) calls newFd(syscall.O_CLOEXEC | syscall.O_NONBLOCK), configureApi(uffdFd, tt.pagesize), and register(uffdFd, memoryStart, uint64(size), UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP) in sequence. The UFFDIO_REGISTER_MODE_MISSING|UFFDIO_REGISTER_MODE_WP combination requires root or CAP_SYS_PTRACE (or vm.unprivileged_userfaultfd=1). On a standard developer machine, newFd() or configureApi() will return EPERM from the kernel ioctl.

Why existing code does not prevent it

Each of the four test functions calls require.NoError(t, err) immediately after configureCrossProcessTest, which calls t.FailNow() on the returned error. There is no t.Skip path, so a non-root run results in a hard test failure rather than an informative skip. By contrast, TestMissing (missing_test.go:17-18) and TestAsyncWriteProtection (async_wp_test.go:33-34) both guard with if os.Geteuid() \!= 0 { t.Skip("skipping test as not running as root") } before any privileged operation.

What the impact would be

A contributor running go test ./pkg/sandbox/uffd/... without root sees four unexplained failures. The error messages (permission denied on an ioctl) are not obviously linked to privilege requirements, so developers spend time diagnosing what appears to be a broken test suite rather than realising they simply need root.

How to fix it

Add "os" to the import block of missing_write_test.go and insert the following guard at the top of each of the four test functions (before the first configureCrossProcessTest call):

if os.Geteuid() \!= 0 {
    t.Skip("skipping test as not running as root")
}

Step-by-step proof

  1. Developer clones repo, runs go test -count=1 -run TestMissingWrite ./pkg/sandbox/uffd/userfaultfd/ as a non-root user.
  2. TestMissingWrite calls configureCrossProcessTest(t, tt).
  3. Inside configureCrossProcessTest, newFd(syscall.O_CLOEXEC | syscall.O_NONBLOCK) performs the userfaultfd(2) syscall — rejected with EPERM since the process lacks CAP_SYS_PTRACE and vm.unprivileged_userfaultfd is 0.
  4. require.NoError(t, err) on the line immediately after calls t.FailNow() with the EPERM error.
  5. Test is reported as FAILED instead of SKIPPED. The same happens for the three parallel/serial variants.
  6. Compare with TestMissing: step 1 is os.Geteuid() \!= 0 which triggers t.Skip — test is reported as SKIPPED with an explanatory message.

accessedOffsets, err := h.offsetsOnce()
require.NoError(t, err)

assert.Equal(t, expectedAccessedOffsets, accessedOffsets, "checking which pages were faulted")

h.checkDirtiness(t, tt.operations)
})
}
}

func TestParallelMissingWrite(t *testing.T) {
t.Parallel()

parallelOperations := 1_000_000
parallelOperations := 10_000

tt := testConfig{
pagesize: header.PageSize,
Expand Down
5 changes: 4 additions & 1 deletion packages/orchestrator/pkg/sandbox/uffd/userfaultfd/userfaultfd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ type Userfaultfd struct {

wg errgroup.Group

// defaultCopyMode overrides the UFFDIO_COPY mode for all faults when non-zero.
defaultCopyMode CULong

logger logger.Logger
}

Expand Down Expand Up @@ -363,7 +366,7 @@ retryLoop:
return fmt.Errorf("failed to read from source after %d attempts: %w", attempt+1, joinedErr)
}

var copyMode CULong
copyMode := u.defaultCopyMode

// Performing copy() on UFFD clears the WP bit unless we explicitly tell
// it not to. We do that for faults caused by a read access. Write accesses
Expand Down
Loading