diff --git a/.github/workflows/govulncheck.yml b/.github/workflows/govulncheck.yml index c6445b1..762ca5a 100644 --- a/.github/workflows/govulncheck.yml +++ b/.github/workflows/govulncheck.yml @@ -1,8 +1,9 @@ name: Vulnerabilities # Known vulnerabilities in the code dbgp calls, in its dependencies and in -# the standard library of the newest Go, which release builds use. Weekly -# too, since new ones are found in code that has not changed. +# the standard library: of the oldest Go that go.mod allows, which go install +# and local builds may use, and of the newest, which release builds use. +# Weekly too, since new ones are found in code that has not changed. on: pull_request: push: @@ -16,11 +17,21 @@ permissions: jobs: govulncheck: runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + go: [minimum, stable] + name: govulncheck (${{ matrix.go }} Go) steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + - if: matrix.go == 'minimum' + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + - if: matrix.go == 'stable' + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version: stable check-latest: true diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 1d47cc3..dbdf73e 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -34,12 +34,20 @@ jobs: run: go vet ./... test: - name: test (PHP ${{ matrix.php }}) - runs-on: ubuntu-latest + # Linux with every PHP version, and Windows with the newest. + name: test (${{ matrix.os == 'windows-latest' && 'Windows, ' || '' }}PHP ${{ matrix.php }}) + runs-on: ${{ matrix.os }} strategy: fail-fast: false matrix: + os: [ubuntu-latest] php: ['8.2', '8.3', '8.4', '8.5'] + include: + - os: windows-latest + php: '8.5' + defaults: + run: + shell: bash steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -47,18 +55,30 @@ jobs: - uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 with: go-version-file: go.mod - # Self-contained PHP interpreter with PHP Debugger compiled in, from the - # latest php-debugger release. + # PHP Debugger, from its latest release: on Linux, a self-contained PHP + # interpreter with it compiled in; on Windows, where the release has only + # the extension, the official PHP build with the extension loaded. - name: Install PHP Debugger env: GH_TOKEN: ${{ github.token }} run: | dir="$RUNNER_TEMP/php-debugger/bin" mkdir -p "$dir" - gh release download --repo php-debugger/php-debugger \ - --pattern "php-php${{ matrix.php }}-nts-linux-x86_64" \ - --output "$dir/php" - chmod +x "$dir/php" + if [ "$RUNNER_OS" = Windows ]; then + curl -fsSL --retry 5 --retry-all-errors -o "$RUNNER_TEMP/php.zip" \ + "https://downloads.php.net/~windows/releases/latest/php-${{ matrix.php }}-nts-Win32-vs17-x64-latest.zip" + unzip -q "$RUNNER_TEMP/php.zip" -d "$dir" + gh release download --repo php-debugger/php-debugger \ + --pattern "php_php-debugger-php${{ matrix.php }}-nts-windows-x64.dll" \ + --output "$dir/ext/php_php_debugger.dll" + # PHP reads the php.ini next to php.exe. + printf 'extension_dir="%s"\nzend_extension=php_debugger\n' "$(cygpath -w "$dir/ext")" > "$dir/php.ini" + else + gh release download --repo php-debugger/php-debugger \ + --pattern "php-php${{ matrix.php }}-nts-linux-x86_64" \ + --output "$dir/php" + chmod +x "$dir/php" + fi echo "$dir" >> "$GITHUB_PATH" - name: Show PHP and debugger versions run: | diff --git a/README.md b/README.md index 6cd6c22..1ff2b87 100644 --- a/README.md +++ b/README.md @@ -81,3 +81,10 @@ php app.php Each script or request that connects becomes a session, stopped at its start. At the prompt, `run` runs to the next breakpoint, `next`, `step` and `out` step through the code, and `stack`, `vars` and `print $user["name"]` show where it stopped. On a terminal it has line editing, tab completion and history. Type `help` at the prompt for all the commands, and see `dbgp -h` for the options, such as `-map` for PHP in a container or on a server. + +## Security + +Like any debugger, dbgp trusts the PHP that connects to it and the agent that uses it: + +- **The PHP port.** DBGp has no authentication: anything that can reach the port, 9003 by default, can connect as PHP and show dbgp made-up code, variables and output, which agents then read. dbgp listens on every address of this machine so that PHP in a container or on another machine can connect, so make sure only your PHP hosts and container networks can reach that port, for example with a firewall. `-idekey` accepts only sessions with that IDE key, which keeps out other PHP, but it is sent in plain text and is not a password. To debug PHP across a network you do not trust, connect through an SSH tunnel or a VPN. `dbgp mcp` only listens while an agent is debugging, and every session shows the address it connected from. +- **The agent.** An agent using dbgp can see everything the debugged script can: its variables, including passwords and keys, and any file PHP can read. `-no-eval` stops the agent from running PHP code of its own, but does not hide anything from it. diff --git a/cmd/dbgp/mcp.go b/cmd/dbgp/mcp.go index 4f928d7..ec7d9eb 100644 --- a/cmd/dbgp/mcp.go +++ b/cmd/dbgp/mcp.go @@ -191,6 +191,8 @@ func runMCP(args []string) int { srv, err := dbgp.NewServer(dbgp.Config{ Addr: phpAddr(*phpPort), IDEKey: *idekey, PathMap: pathMap, ClaimTimeout: claimTimeout, + // The tools never read the engine mapping that the probe reports. + NoEval: *noEval, }) if err != nil { fmt.Fprintln(os.Stderr, err) @@ -352,9 +354,12 @@ type tools struct { // allowCode allows what runs the agent's PHP code in the debugged script. allowCode bool - // mu guards agents, by agent id: see agentID. + // mu guards agents, by agent id (see agentID), and detaching. mu sync.Mutex agents map[string]*agentState + // detaching holds the ids of the sessions being detached because the + // agents that took them left: no agent can take them meanwhile. + detaching map[int]bool } // agentState is what the tools keep for one agent. Agents sharing the @@ -402,33 +407,53 @@ func (a *agentState) take(id int) { // scripts run on instead of holding their PHP workers, unless another agent // has taken them too. func (t *tools) leave(id string) error { + orphans, err := t.release(id) + t.detachSessions(orphans) + return err +} + +// release forgets the agent with the given id, stops listening if it was +// the last agent listening, and returns the live sessions only it took, +// marked as detaching, so no other agent takes one before it is detached. +func (t *tools) release(id string) ([]*dbgp.Session, error) { t.mu.Lock() + defer t.mu.Unlock() a := t.agents[id] if a == nil { - t.mu.Unlock() - return nil + return nil, nil } delete(t.agents, id) - var orphans []int + var orphans []*dbgp.Session for sid := range a.taken { - if !t.taken(sid) { - orphans = append(orphans, sid) + if t.taken(sid) { + continue + } + if s := t.srv.Session(sid); s != nil && !ended(s.State()) { + if t.detaching == nil { + t.detaching = map[int]bool{} + } + t.detaching[sid] = true + orphans = append(orphans, s) } } - var err error if a.listening && t.listeners() == 0 { - err = t.srv.StopListening() + return orphans, t.srv.StopListening() } - t.mu.Unlock() + return orphans, nil +} - // Outside t.mu: detaching a stopped session waits for PHP's answer. - for _, sid := range orphans { - if s := t.srv.Session(sid); s != nil && !ended(s.State()) { - _ = s.Detach() - _ = s.Close() - } +// detachSessions detaches the sessions release returned, outside t.mu: +// detaching a stopped session waits for PHP's answer. Once they have ended, +// agents can name them again, to read their output. +func (t *tools) detachSessions(sessions []*dbgp.Session) { + for _, s := range sessions { + _ = s.Detach() + _ = s.Close() + <-s.Done() + t.mu.Lock() + delete(t.detaching, s.ID()) + t.mu.Unlock() } - return err } // taken reports whether any agent has taken session id. t.mu must be held. diff --git a/cmd/dbgp/mcp_daemon.go b/cmd/dbgp/mcp_daemon.go index 6dd137c..1b3fee7 100644 --- a/cmd/dbgp/mcp_daemon.go +++ b/cmd/dbgp/mcp_daemon.go @@ -18,6 +18,7 @@ import ( "os/exec" "path/filepath" "strconv" + "sync" "time" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -375,7 +376,11 @@ func bridge(ctx context.Context, st *daemonState) error { if err != nil { return err } - defer leaveServer(client, st.URL) + presence, err := attachServer(ctx, client, st.URL) + if err != nil { + return err + } + defer presence.Close() remote, err := (&mcp.StreamableClientTransport{Endpoint: st.URL, HTTPClient: client}).Connect(ctx) if err != nil { return err @@ -404,36 +409,40 @@ func bridge(ctx context.Context, st *daemonState) error { return nil } -// leavePath is where dbgp mcp tells the server its agent has gone, so it -// no longer keeps dbgp listening. -const leavePath = "/leave" - -// leaveTimeout is how long dbgp mcp waits for the server to hear it has -// gone. -const leaveTimeout = 2 * time.Second +// attachPath is where dbgp mcp tells the server its agent is there, for as +// long as it runs: it keeps the request open, and the server takes the +// agent as gone when the request ends. The operating system ends it when +// dbgp mcp ends, even when it crashes or is killed. +const attachPath = "/attach" -// leaveServer tells the server at rawURL, through client, that the agent -// client sends the id of has gone. It is best effort: the agent has -// already gone, and so has its dbgp mcp when the server cannot be reached. -func leaveServer(client *http.Client, rawURL string) { +// attachServer tells the server at rawURL, through client, that the agent +// client sends the id of is there, until the returned body is closed or +// ctx is done. +func attachServer(ctx context.Context, client *http.Client, rawURL string) (io.Closer, error) { u, err := url.Parse(rawURL) if err != nil { - return + return nil, err } - u.Path = leavePath - ctx, cancel := context.WithTimeout(context.Background(), leaveTimeout) - defer cancel() + u.Path = attachPath req, err := http.NewRequestWithContext(ctx, http.MethodPost, u.String(), nil) if err != nil { - return + return nil, err } - if resp, err := client.Do(req); err == nil { + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("attach to the background server: %w", err) + } + if resp.StatusCode != http.StatusOK { _ = resp.Body.Close() + return nil, fmt.Errorf("attach to the background server: %s", resp.Status) } + return resp.Body, nil } -// leave forgets the agent each request is from, by its agentHeader. -func leave(t *tools) http.Handler { +// attach holds each request open until the agent it is from, by its +// agentHeader, has gone, then forgets that agent and closes its MCP +// sessions. +func attach(t *tools, sessions *mcpSessions) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { if req.Method != http.MethodPost { w.Header().Set("Allow", http.MethodPost) @@ -445,14 +454,72 @@ func leave(t *tools) http.Handler { http.Error(w, "no "+agentHeader+" header", http.StatusBadRequest) return } + w.WriteHeader(http.StatusOK) + _ = http.NewResponseController(w).Flush() + // Done when dbgp mcp closes the connection or the server stops. + <-req.Context().Done() if err := t.leave(id); err != nil { - http.Error(w, err.Error(), http.StatusInternalServerError) - return + fmt.Fprintf(os.Stderr, "dbgp: agent left: %v\n", err) } - w.WriteHeader(http.StatusNoContent) + sessions.close(id) + }) +} + +// mcpSessionHeader carries the id of the MCP session a request belongs to, +// which the MCP library assigns in its answer to initialize. +const mcpSessionHeader = "Mcp-Session-Id" + +// mcpSessions records which MCP sessions each agent uses, to close them when +// it leaves: the MCP library keeps a session until its client ends it, which +// a dbgp mcp that crashed or was killed never does. +type mcpSessions struct { + server *mcp.Server + + mu sync.Mutex + byAgent map[string]map[string]bool // session ids, by agent id +} + +// track passes requests on to next, recording the MCP session each is from +// for the agent it is from. +func (m *mcpSessions) track(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + agent := req.Header.Get(agentHeader) + m.add(agent, req.Header.Get(mcpSessionHeader)) + next.ServeHTTP(w, req) + // The answer to initialize, which the session starts with. + m.add(agent, w.Header().Get(mcpSessionHeader)) }) } +func (m *mcpSessions) add(agent, session string) { + if agent == "" || session == "" { + return + } + m.mu.Lock() + defer m.mu.Unlock() + if m.byAgent == nil { + m.byAgent = map[string]map[string]bool{} + } + if m.byAgent[agent] == nil { + m.byAgent[agent] = map[string]bool{} + } + m.byAgent[agent][session] = true +} + +// close closes the MCP sessions of the agent with the given id, which has +// left, and forgets them. +func (m *mcpSessions) close(agent string) { + m.mu.Lock() + ids := m.byAgent[agent] + delete(m.byAgent, agent) + m.mu.Unlock() + for ss := range m.server.Sessions() { + if ids[ss.ID()] { + _ = ss.Close() + } + } +} + // agentHeader carries the id of the agent a request is from, which its // dbgp mcp chooses: see agentID. const agentHeader = "Dbgp-Agent" @@ -526,14 +593,15 @@ func serveHTTP(ctx context.Context, server *mcp.Server, t *tools, port int) erro return nil } -// httpHandler serves MCP at /mcp and agents leaving, to requests with st's +// httpHandler serves MCP at /mcp and agents' presence, to requests with st's // token, and the identity check. t are server's tools. func httpHandler(server *mcp.Server, t *tools, st daemonState) http.Handler { handler := mcp.NewStreamableHTTPHandler(func(*http.Request) *mcp.Server { return server }, &mcp.StreamableHTTPOptions{CrossOriginProtection: http.NewCrossOriginProtection()}) + sessions := &mcpSessions{server: server} mux := http.NewServeMux() - mux.Handle("/mcp", requireToken(st.Token, handler)) - mux.Handle(leavePath, requireToken(st.Token, leave(t))) + mux.Handle("/mcp", requireToken(st.Token, sessions.track(handler))) + mux.Handle(attachPath, requireToken(st.Token, attach(t, sessions))) mux.Handle(identifyPath, identify(st)) return mux } diff --git a/cmd/dbgp/mcp_daemon_test.go b/cmd/dbgp/mcp_daemon_test.go index 680c714..c510266 100644 --- a/cmd/dbgp/mcp_daemon_test.go +++ b/cmd/dbgp/mcp_daemon_test.go @@ -212,9 +212,9 @@ func TestMCPBridge(t *testing.T) { if err := bridged.Close(); err != nil { t.Errorf("close: %v", err) } - // Its dbgp mcp told the server the agent left, which counts as unlisten. - callTool(t, direct, "status", nil, &status) - if status.Listening { + // Its dbgp mcp ending tells the server the agent left, which counts as + // unlisten. + if !stopsListening(t, direct) { t.Error("still listening after the only agent that called listen left") } if !strings.Contains(stderr.String(), "using the background server at "+url) || !strings.Contains(stderr.String(), "-listen ignored") { @@ -224,6 +224,48 @@ func TestMCPBridge(t *testing.T) { callTool(t, direct, "status", nil, &status) } +// stopsListening reports whether the server cs is connected to stops +// listening within 5s. +func stopsListening(t *testing.T, cs *mcp.ClientSession) bool { + t.Helper() + for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(20 * time.Millisecond) { + var status statusOutput + callTool(t, cs, "status", nil, &status) + if !status.Listening { + return true + } + } + return false +} + +// An agent's dbgp mcp that is killed, so it cannot say it is leaving, +// still leaves: the operating system closes its connection to the server. +func TestMCPBridgeKilled(t *testing.T) { + bin := buildBinary(t) + daemonEnv(t) + url := startTestDaemon(t, bin) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, bin, "mcp") + bridged, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil). + Connect(ctx, &mcp.CommandTransport{Command: cmd}, nil) + if err != nil { + t.Fatal(err) + } + defer bridged.Close() + direct := connectDirect(ctx, t, url) + defer direct.Close() + + callTool(t, bridged, "listen", nil, &listenOutput{}) + if err := cmd.Process.Kill(); err != nil { + t.Fatal(err) + } + if !stopsListening(t, direct) { + t.Error("still listening after the only agent that called listen was killed") + } +} + // connectDirect connects to the background server over HTTP, with the token // from the state file, as dbgp mcp does. func connectDirect(ctx context.Context, t *testing.T, url string) *mcp.ClientSession { diff --git a/cmd/dbgp/mcp_inspect.go b/cmd/dbgp/mcp_inspect.go index 40996da..66a47d8 100644 --- a/cmd/dbgp/mcp_inspect.go +++ b/cmd/dbgp/mcp_inspect.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "math" "unicode/utf8" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -37,7 +36,8 @@ func (t *tools) addInspectTools(server *mcp.Server) { mcp.AddTool(server, &mcp.Tool{ Name: "variable_value", Description: "Show the whole value of a variable whose value variables or variable returned truncated, " + - "such as a long string. A value longer than 32 KB is returned in parts: call again with offset set to next.", + "such as a long string. A value longer than 32 KB is returned in parts: call again with offset set to next. " + + "Only the first 20 MB of a value can be read: next is not set past that.", Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, }, t.variableValue) } @@ -220,6 +220,11 @@ type variableValueOutput struct { Next int `json:"next,omitempty" jsonschema:"where the rest of the value starts: call variable_value again with this offset; absent when the value is complete"` } +// maxValueOffset is the furthest variable_value can start a part: PHP sends +// a value's first bytes up to the end of the part, and at most +// dbgp.MaxValueBytes of them. +const maxValueOffset = dbgp.MaxValueBytes - maxTextBytes - utf8.UTFMax + func (t *tools) variableValue(_ context.Context, req *mcp.CallToolRequest, in variableValueInput) (*mcp.CallToolResult, variableValueOutput, error) { s, err := t.session(req, in.Session) if err != nil { @@ -228,9 +233,13 @@ func (t *tools) variableValue(_ context.Context, req *mcp.CallToolRequest, in va if in.Name == "" { return nil, variableValueOutput{}, errors.New("name is required, e.g. $html") } - if in.Offset < 0 || in.Offset > math.MaxInt32 { + if in.Offset < 0 { return nil, variableValueOutput{}, fmt.Errorf("offset must be from 0 to the size of %s", in.Name) } + if in.Offset > maxValueOffset { + return nil, variableValueOutput{}, fmt.Errorf("only the first %d MB of a value can be read: offset must be at most %d", + dbgp.MaxValueBytes>>20, maxValueOffset) + } // The engine sends a value's first bytes, not a range: ask for those up // to the end of this part, not the whole value each time, and a few more // so that the part can end on a whole character. @@ -243,7 +252,7 @@ func (t *tools) variableValue(_ context.Context, req *mcp.CallToolRequest, in va return nil, variableValueOutput{}, fmt.Errorf("offset must be from 0 to %d, the size of %s", size, in.Name) } out := variableValueOutput{Name: in.Name, Value: cutText(value[in.Offset:], maxTextBytes), Size: size} - if end := in.Offset + len(out.Value); end < size { + if end := in.Offset + len(out.Value); end < size && end <= maxValueOffset { out.Next = end } return nil, out, nil diff --git a/cmd/dbgp/mcp_inspect_test.go b/cmd/dbgp/mcp_inspect_test.go index ccfa499..33deb7f 100644 --- a/cmd/dbgp/mcp_inspect_test.go +++ b/cmd/dbgp/mcp_inspect_test.go @@ -1,6 +1,7 @@ package main import ( + "os" "path/filepath" "strings" "testing" @@ -210,3 +211,35 @@ func TestMCPInspectRunningSession(t *testing.T) { } } } + +// variable_value reads up to dbgp.MaxValueBytes of a value, which fits in a +// packet; past that it says so, rather than ask for more than fits. +func TestMCPVariableValueLimit(t *testing.T) { + requireEngine(t) + script := filepath.Join(t.TempDir(), "big.php") + if err := os.WriteFile(script, []byte(" 2"}, breakpointInfo{ID: 3, Type: "conditional", File: "/app/a.php", Line: 5, Condition: "$i > 2"}}, + {map[string]any{"file": "/app/a.php", "line": 12}, breakpointInfo{ID: 1, Type: "line", File: absPath(t, "/app/a.php"), Line: 12}}, + {map[string]any{"type": "line", "file": "/app/../app/b.php", "line": 3}, breakpointInfo{ID: 2, Type: "line", File: absPath(t, "/app/b.php"), Line: 3}}, + {map[string]any{"file": "/app/a.php", "line": 5, "condition": "$i > 2"}, breakpointInfo{ID: 3, Type: "conditional", File: absPath(t, "/app/a.php"), Line: 5, Condition: "$i > 2"}}, {map[string]any{"type": "call", "function": "add"}, breakpointInfo{ID: 4, Type: "call", Function: "add"}}, {map[string]any{"type": "return", "function": "add"}, breakpointInfo{ID: 5, Type: "return", Function: "add"}}, {map[string]any{"type": "exception", "exception": "RuntimeException"}, breakpointInfo{ID: 6, Type: "exception", Exception: "RuntimeException"}}, @@ -454,7 +472,7 @@ func TestMCPPathMappings(t *testing.T) { callTool(t, cs, "add_path_mapping", map[string]any{"local": "/home/me/app", "remote": "/var/www/html"}, &out) callTool(t, cs, "add_path_mapping", map[string]any{"local": "/srv/../srv/php", "remote": "/srv/php"}, &out) callTool(t, cs, "add_path_mapping", map[string]any{"local": "/home/me/app/", "remote": "/app"}, &out) - want := []pathMapping{{Local: "/srv/php", Remote: "/srv/php"}, {Local: "/home/me/app", Remote: "/app"}} + want := []pathMapping{{Local: absPath(t, "/srv/php"), Remote: "/srv/php"}, {Local: absPath(t, "/home/me/app"), Remote: "/app"}} if fmt.Sprint(out.PathMappings) != fmt.Sprint(want) || out.Warning != "" { t.Errorf("add_path_mapping = %+v, want %+v", out, want) } @@ -545,3 +563,56 @@ func TestMCPListenWarnings(t *testing.T) { t.Errorf("listen = %+v, want port %s with a warning about [::1]", out, port) } } + +// dbgp mcp -no-eval sends no eval of its own to the engine: the setup +// probe for the engine's path mapping is skipped. +func TestMCPNoEvalSendsNoEval(t *testing.T) { + bin := buildBinary(t) + daemonEnv(t) + init, err := os.ReadFile("../../testdata/dbgp/init.xml") + if err != nil { + t.Fatal(err) + } + for _, noEval := range []bool{false, true} { + args := []string{"mcp", "-php-port", "0"} + if noEval { + args = append(args, "-no-eval") + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + cs, err := mcp.NewClient(&mcp.Implementation{Name: "test", Version: "0"}, nil). + Connect(ctx, &mcp.CommandTransport{Command: exec.CommandContext(ctx, bin, args...)}, nil) + if err != nil { + t.Fatal(err) + } + defer cs.Close() + var listening listenOutput + callTool(t, cs, "listen", nil, &listening) + + // An engine that answers every setup command, until output capture, + // the last one. + conn, err := net.Dial("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(listening.Port))) + if err != nil { + t.Fatal(err) + } + defer conn.Close() + _ = conn.SetDeadline(time.Now().Add(10 * time.Second)) + fmt.Fprintf(conn, "%d\x00%s\x00", len(init), init) + reader := bufio.NewReader(conn) + var commands []string + for !slices.Contains(commands, "stdout") { + line, err := reader.ReadString(0) + if err != nil { + t.Fatalf("%v: reading commands %v: %v", args, commands, err) + } + fields := strings.Fields(strings.TrimSuffix(line, "\x00")) + id := fields[slices.Index(fields, "-i")+1] + commands = append(commands, fields[0]) + resp := fmt.Sprintf(``, fields[0], id) + fmt.Fprintf(conn, "%d\x00%s\x00", len(resp), resp) + } + if sent := slices.Contains(commands, "eval"); sent == noEval { + t.Errorf("%v: setup commands %v, eval sent %v", args, commands, sent) + } + } +} diff --git a/cmd/dbgp/repl.go b/cmd/dbgp/repl.go index d4fc9b4..eb9b0de 100644 --- a/cmd/dbgp/repl.go +++ b/cmd/dbgp/repl.go @@ -900,6 +900,7 @@ func (r *repl) sourceLines(s *dbgp.Session, file string, line, current int) (str } var b strings.Builder for i, text := range strings.Split(strings.TrimRight(src, "\n"), "\n") { + text = strings.TrimSuffix(text, "\r") // Windows line endings n := begin + i mark := " " if n == current { diff --git a/cmd/dbgp/repl_test.go b/cmd/dbgp/repl_test.go index 98bc9c9..5de5ef0 100644 --- a/cmd/dbgp/repl_test.go +++ b/cmd/dbgp/repl_test.go @@ -39,7 +39,7 @@ func newTestREPL(t *testing.T) (*repl, *dbgp.Server, func() string) { func TestParsePathMap(t *testing.T) { abs, _ := filepath.Abs("rel") got, err := parsePathMap([]string{"/home/me/app=/var/www", "/a=C:/b", "rel=/srv"}) - want := []dbgp.PathMapping{{Local: "/home/me/app", Remote: "/var/www"}, {Local: "/a", Remote: "C:/b"}, {Local: abs, Remote: "/srv"}} + want := []dbgp.PathMapping{{Local: absPath(t, "/home/me/app"), Remote: "/var/www"}, {Local: absPath(t, "/a"), Remote: "C:/b"}, {Local: abs, Remote: "/srv"}} if err != nil || !reflect.DeepEqual(got, want) { t.Errorf("parsePathMap = %+v, %v", got, err) } @@ -57,9 +57,9 @@ func TestParseBreakpoint(t *testing.T) { args string want dbgp.Breakpoint }{ - {"/app/a.php:12", dbgp.Breakpoint{Type: "line", File: "/app/a.php", Line: 12}}, + {"/app/a.php:12", dbgp.Breakpoint{Type: "line", File: absPath(t, "/app/a.php"), Line: 12}}, {"a.php:3", dbgp.Breakpoint{Type: "line", File: abs, Line: 3}}, - {"/app/a.php:5 if $i > 2", dbgp.Breakpoint{Type: "conditional", File: "/app/a.php", Line: 5, Condition: "$i > 2"}}, + {"/app/a.php:5 if $i > 2", dbgp.Breakpoint{Type: "conditional", File: absPath(t, "/app/a.php"), Line: 5, Condition: "$i > 2"}}, {"call App\\run", dbgp.Breakpoint{Type: "call", Function: "App\\run"}}, {"return add", dbgp.Breakpoint{Type: "return", Function: "add"}}, {"exception RuntimeException", dbgp.Breakpoint{Type: "exception", Exception: "RuntimeException"}}, @@ -447,13 +447,7 @@ func TestREPLBreakpointKindsAndDetach(t *testing.T) { // The built command: flags, startup and quitting. func TestBinary(t *testing.T) { - if testing.Short() { - t.Skip("builds the binary") - } - bin := filepath.Join(t.TempDir(), "dbgp") - if out, err := exec.Command("go", "build", "-o", bin, ".").CombinedOutput(); err != nil { - t.Fatalf("go build: %v\n%s", err, out) - } + bin := buildBinary(t) out, _ := exec.Command(bin, "-h").CombinedOutput() if !strings.Contains(string(out), "Usage: dbgp [options]") || !strings.Contains(string(out), "-map LOCAL=REMOTE") { @@ -485,7 +479,7 @@ func TestBinary(t *testing.T) { cmd.Stdin = strings.NewReader("breaks\nquit\n") out, err = cmd.CombinedOutput() if err != nil || !strings.Contains(string(out), "Listening for PHP on port ") || - !strings.Contains(string(out), "1 /app/a.php:3\n") || !strings.Contains(string(out), "2 /app/b.php:5\n") { + !strings.Contains(string(out), "1 "+absPath(t, "/app/a.php")+":3\n") || !strings.Contains(string(out), "2 "+absPath(t, "/app/b.php")+":5\n") { t.Errorf("session: %v\n%s", err, out) } } @@ -577,17 +571,18 @@ func TestMapAndMaps(t *testing.T) { r.execLine("maps") abs, _ := filepath.Abs("testdata/php") - want := []dbgp.PathMapping{{Local: abs, Remote: "/srv/php"}, {Local: "/home/me/app", Remote: "/app"}} + app := absPath(t, "/home/me/app") + want := []dbgp.PathMapping{{Local: abs, Remote: "/srv/php"}, {Local: app, Remote: "/app"}} if got := srv.PathMap(); !reflect.DeepEqual(got, want) { t.Errorf("PathMap() = %+v, want %+v", got, want) } out := output() for _, line := range []string{ "No path mappings.\n", - "Mapped /home/me/app => /var/www/html\n", + "Mapped " + app + " => /var/www/html\n", "Mapped " + abs + " => /srv/php\n", "Error: usage: map LOCAL=REMOTE\n", - abs + " => /srv/php\n/home/me/app => /app\n", + abs + " => /srv/php\n" + app + " => /app\n", } { if !strings.Contains(out, line) { t.Errorf("output lacks %q", line) diff --git a/go.mod b/go.mod index eb0bdb7..b1b80e5 100644 --- a/go.mod +++ b/go.mod @@ -1,11 +1,13 @@ module github.com/php-debugger/dbgp-client -go 1.26.0 +go 1.26.10 require ( github.com/ergochat/readline v0.1.3 github.com/google/jsonschema-go v0.4.3 github.com/modelcontextprotocol/go-sdk v1.8.0 + golang.org/x/sync v0.24.0 + golang.org/x/text v0.43.0 ) require ( @@ -13,8 +15,6 @@ require ( github.com/segmentio/encoding v0.5.4 // indirect github.com/yosida95/uritemplate/v3 v3.0.2 // indirect golang.org/x/oauth2 v0.35.0 // indirect - golang.org/x/sync v0.24.0 // indirect golang.org/x/sys v0.44.0 // indirect - golang.org/x/text v0.43.0 // indirect golang.org/x/time v0.15.0 // indirect ) diff --git a/integration_test.go b/integration_test.go index 8f6bdbb..4095834 100644 --- a/integration_test.go +++ b/integration_test.go @@ -7,6 +7,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" "time" @@ -375,6 +376,11 @@ func TestEnginePathMapping(t *testing.T) { // detects it and passes local paths through. func TestEngineOwnPathMapping(t *testing.T) { requireDebugEngine(t) + if runtime.GOOS == "windows" { + // PHP Debugger reports path mapping as enabled there, but applies + // no map file. + t.Skip("PHP Debugger does not apply map files on Windows") + } src, err := os.ReadFile(debuggeePath(t)) if err != nil { t.Fatal(err) @@ -386,7 +392,7 @@ func TestEngineOwnPathMapping(t *testing.T) { } remoteScript := filepath.Join(remoteDir, "basic.php") localDir := "/home/agent/project" - script := localDir + "/basic.php" + script := filepath.FromSlash(localDir + "/basic.php") files := map[string]string{ remoteScript: string(src), filepath.Join(remoteDir, ".xdebug", "agent.map"): "remote_prefix: " + filepath.ToSlash(remoteDir) + diff --git a/listen.go b/listen.go new file mode 100644 index 0000000..6f3ddd0 --- /dev/null +++ b/listen.go @@ -0,0 +1,27 @@ +package dbgp + +import ( + "context" + "net" +) + +// listenTCP listens on addr for PHP. +func listenTCP(addr string) (net.Listener, error) { + lc := net.ListenConfig{Control: listenControl} + return lc.Listen(context.Background(), "tcp", addr) +} + +// localIPs returns this machine's addresses, except link-local ones, +// always including the loopback addresses. +func localIPs() []net.IP { + ips := []net.IP{net.IPv4(127, 0, 0, 1), net.IPv6loopback} + addrs, _ := net.InterfaceAddrs() + for _, a := range addrs { + ipNet, ok := a.(*net.IPNet) + if !ok || ipNet.IP.IsLinkLocalUnicast() || ipNet.IP.IsLoopback() { + continue + } + ips = append(ips, ipNet.IP) + } + return ips +} diff --git a/listen_darwin.go b/listen_darwin.go index a995029..2715307 100644 --- a/listen_darwin.go +++ b/listen_darwin.go @@ -34,21 +34,6 @@ func shadowedAddr(l net.Listener) string { return "" } -// localIPs returns this machine's addresses, except link-local ones, -// always including the loopback addresses. -func localIPs() []net.IP { - ips := []net.IP{net.IPv4(127, 0, 0, 1), net.IPv6loopback} - addrs, _ := net.InterfaceAddrs() - for _, a := range addrs { - ipNet, ok := a.(*net.IPNet) - if !ok || ipNet.IP.IsLinkLocalUnicast() || ipNet.IP.IsLoopback() { - continue - } - ips = append(ips, ipNet.IP) - } - return ips -} - // bindInUse reports whether binding a TCP socket to ip and port fails // because another socket is bound there. The socket is only bound, never // listening, so no connection reaches it. diff --git a/listen_nonwindows.go b/listen_nonwindows.go new file mode 100644 index 0000000..ae219ec --- /dev/null +++ b/listen_nonwindows.go @@ -0,0 +1,8 @@ +//go:build !windows + +package dbgp + +import "syscall" + +// listenControl is only needed on Windows. +var listenControl func(network, address string, c syscall.RawConn) error diff --git a/listen_other.go b/listen_other.go index 94090d1..7267ff8 100644 --- a/listen_other.go +++ b/listen_other.go @@ -5,5 +5,6 @@ package dbgp import "net" // shadowedAddr is only needed on macOS: Linux refuses to listen on every -// address when another program listens on one of them. +// address when another program listens on one of them, and so does +// Windows with the exclusive addresses dbgp asks for. func shadowedAddr(net.Listener) string { return "" } diff --git a/listen_darwin_test.go b/listen_test.go similarity index 52% rename from listen_darwin_test.go rename to listen_test.go index fe2635f..a7bbe1b 100644 --- a/listen_darwin_test.go +++ b/listen_test.go @@ -2,13 +2,24 @@ package dbgp import ( "net" + "runtime" "strings" "testing" ) -// macOS lets a server listen on every address while another program -// listens on one of them, which would then get the connections to it: -// StartListening refuses, as Linux does. +// shadowedError returns what StartListening's error says when another +// program listens on addr: dbgp's own message on macOS, which would let +// dbgp listen, and "" elsewhere, where the system refuses by itself. +func shadowedError(addr string) string { + if runtime.GOOS == "darwin" { + return "another program is listening on " + addr + } + return "" +} + +// StartListening refuses to listen on every address while another program +// listens on one of them, which would then get the connections to it. +// Linux refuses by itself; macOS does not, so dbgp checks. func TestListenRefusesShadowedAddress(t *testing.T) { hosts := []string{"127.0.0.1", "::1"} for _, ip := range localIPs() { @@ -28,7 +39,7 @@ func TestListenRefusesShadowedAddress(t *testing.T) { t.Fatal(err) } err = srv.StartListening() - want := "another program is listening on " + net.JoinHostPort(host, port) + want := shadowedError(net.JoinHostPort(host, port)) if err == nil || !strings.Contains(err.Error(), want) { t.Errorf("with %s taken: StartListening = %v, want %q", host, err, want) } @@ -58,8 +69,39 @@ func TestListenOnIPv4AnyRefusesShadowedIPv6(t *testing.T) { t.Fatal(err) } defer srv.Close() - want := "another program is listening on [::1]:" + port + want := shadowedError("[::1]:" + port) if err := srv.StartListening(); err == nil || !strings.Contains(err.Error(), want) { t.Errorf("StartListening = %v, want %q", err, want) } } + +// dbgp can listen on its port again at once after it stops listening, even +// while connections it closed itself still linger on the port (TIME_WAIT), +// as after debugging a few requests. +func TestListenAgainAfterClosingConnections(t *testing.T) { + for _, host := range []string{"", "127.0.0.1"} { + l, err := listenTCP(net.JoinHostPort(host, "0")) + if err != nil { + t.Fatal(err) + } + _, port, _ := net.SplitHostPort(l.Addr().String()) + client, err := net.Dial("tcp", net.JoinHostPort("127.0.0.1", port)) + if err != nil { + t.Fatal(err) + } + conn, err := l.Accept() + if err != nil { + t.Fatal(err) + } + _ = conn.Close() // dbgp's side closes first, so it lingers + _, _ = client.Read(make([]byte, 1)) + _ = client.Close() + _ = l.Close() + + l, err = listenTCP(net.JoinHostPort(host, port)) + if err != nil { + t.Fatalf("listening on %q again: %v", host, err) + } + _ = l.Close() + } +} diff --git a/listen_windows.go b/listen_windows.go new file mode 100644 index 0000000..c5b5aa7 --- /dev/null +++ b/listen_windows.go @@ -0,0 +1,22 @@ +//go:build windows + +package dbgp + +import "syscall" + +// soExclusiveAddrUse is Windows' SO_EXCLUSIVEADDRUSE, which syscall lacks. +const soExclusiveAddrUse = ^syscall.SO_REUSEADDR + +// listenControl makes the addresses dbgp listens on its own. Windows +// otherwise lets dbgp listen on every address while another program +// listens on one of them, which then gets the connections to it, and lets +// another program listen on one of dbgp's addresses later. +func listenControl(_, _ string, c syscall.RawConn) error { + var err error + if cerr := c.Control(func(fd uintptr) { + err = syscall.SetsockoptInt(syscall.Handle(fd), syscall.SOL_SOCKET, soExclusiveAddrUse, 1) + }); cerr != nil { + return cerr + } + return err +} diff --git a/pathmap.go b/pathmap.go index c54bf52..91d2b53 100644 --- a/pathmap.go +++ b/pathmap.go @@ -4,9 +4,15 @@ import ( "fmt" "net/url" "path/filepath" + "runtime" "strings" ) +// localIgnoresCase reports whether local paths are compared regardless of +// case: on Windows, and on macOS, whose disks ignore case unless they were +// formatted otherwise. It is a variable so tests can change it. +var localIgnoresCase = runtime.GOOS == "windows" || runtime.GOOS == "darwin" + // PathMapping maps a local directory to the directory the engine sees, for // PHP running in a container or on a remote server. type PathMapping struct { @@ -52,12 +58,12 @@ func (m PathMap) ToRemote(local string) string { local = filepath.Clean(local) best, rest := -1, "" for i, pm := range m { - if r, ok := cutPathPrefix(local, pm.Local, string(filepath.Separator)); ok && (best < 0 || len(pm.Local) > len(m[best].Local)) { + if r, ok := cutPathPrefix(local, pm.Local, string(filepath.Separator), localIgnoresCase); ok && (best < 0 || len(pm.Local) > len(m[best].Local)) { best, rest = i, r } } if best < 0 { - return local + return filepath.ToSlash(local) } return joinPath(m[best].Remote, filepath.ToSlash(rest), "/") } @@ -67,7 +73,7 @@ func (m PathMap) ToLocal(remote string) string { remote = cleanRemote(remote) best, rest := -1, "" for i, pm := range m { - if r, ok := cutPathPrefix(remote, pm.Remote, "/"); ok && (best < 0 || len(pm.Remote) > len(m[best].Remote)) { + if r, ok := cutPathPrefix(remote, pm.Remote, "/", false); ok && (best < 0 || len(pm.Remote) > len(m[best].Remote)) { best, rest = i, r } } @@ -79,18 +85,38 @@ func (m PathMap) ToLocal(remote string) string { // cutPathPrefix reports whether p is prefix or inside it, and returns the // remainder. Matching is by whole path components: /var/www does not -// contain /var/www2. -func cutPathPrefix(p, prefix, sep string) (string, bool) { - if p == prefix { +// contain /var/www2. It ignores case if ignoreCase is set, and for a Windows +// prefix, as Windows does: C:\Project contains c:\project\a.php. +func cutPathPrefix(p, prefix, sep string, ignoreCase bool) (string, bool) { + if samePath(p, prefix, ignoreCase) { return "", true } if !strings.HasSuffix(prefix, sep) { prefix += sep } - if rest, ok := strings.CutPrefix(p, prefix); ok { - return rest, true + if len(p) < len(prefix) || !samePath(p[:len(prefix)], prefix, ignoreCase) { + return "", false + } + return p[len(prefix):], true +} + +// samePath reports whether paths a and b are the same: regardless of case +// if ignoreCase is set or for Windows paths, exactly otherwise. +func samePath(a, b string, ignoreCase bool) bool { + if ignoreCase || windowsPath(a) || windowsPath(b) { + return strings.EqualFold(a, b) + } + return a == b +} + +// windowsPath reports whether p is a Windows path, which Windows compares +// regardless of case: one that starts with a drive, such as C:\dir or +// C:/dir, or a UNC share, such as \\server\share or //server/share. +func windowsPath(p string) bool { + if len(p) >= 2 && p[1] == ':' && 'a' <= p[0]|0x20 && p[0]|0x20 <= 'z' { + return true } - return "", false + return strings.HasPrefix(p, `\\`) || strings.HasPrefix(p, "//") } func joinPath(dir, rest, sep string) string { diff --git a/pathmap_test.go b/pathmap_test.go index 66ef4f4..9421d16 100644 --- a/pathmap_test.go +++ b/pathmap_test.go @@ -50,7 +50,7 @@ func TestPathMapTranslation(t *testing.T) { } // Mappings match whole path components only. - if got := m.ToRemote(filepath.FromSlash("/home/me/project2/a.php")); got != filepath.FromSlash("/home/me/project2/a.php") { + if got := m.ToRemote(filepath.FromSlash("/home/me/project2/a.php")); got != "/home/me/project2/a.php" { t.Errorf("ToRemote matched a partial component: %q", got) } if got := m.ToLocal("/var/www/html2/a.php"); got != filepath.FromSlash("/var/www/html2/a.php") { @@ -282,7 +282,7 @@ func TestAddPathMapping(t *testing.T) { if got := e.LastCommand("breakpoint_set").Args["f"]; got != "file:///home/me/app/basic.php" { t.Fatalf("breakpoint before mapping sent for %q", got) } - if sess.Script() != "/app/basic.php" { + if sess.Script() != filepath.FromSlash("/app/basic.php") { t.Fatalf("Script() before mapping = %q", sess.Script()) } e.ClearReceived() @@ -358,3 +358,116 @@ func TestAddPathMappingWhileRunning(t *testing.T) { t.Errorf("commands = %q, want %q", strings.Join(names, " "), want) } } + +// Windows compares paths regardless of case, so mappings for Windows paths +// do too, on either side; other paths are compared exactly. +func TestPathMapWindowsCase(t *testing.T) { + m := mustPathMap(t, + PathMapping{Local: "/home/me/win", Remote: `C:\Inetpub\wwwroot`}, + PathMapping{Local: "/home/me/share", Remote: "//FileServer/php"}, + PathMapping{Local: "/home/me/project", Remote: "/var/www/html"}, + ) + tests := []struct{ remote, local string }{ + // PHP on Windows may send a path in another case: what follows the + // mapped directory keeps its own. + {"c:/inetpub/WWWROOT/Default.php", "/home/me/win/Default.php"}, + {`C:\INETPUB\wwwroot`, "/home/me/win"}, + {"//fileserver/PHP/app.php", "/home/me/share/app.php"}, + // Linux paths are case-sensitive. + {"/VAR/www/html/a.php", "/VAR/www/html/a.php"}, + // Whole components only, in any case. + {"C:/inetpub/wwwroot2/a.php", "C:/inetpub/wwwroot2/a.php"}, + } + for _, tt := range tests { + if got := m.ToLocal(tt.remote); got != filepath.FromSlash(tt.local) { + t.Errorf("ToLocal(%q) = %q, want %q", tt.remote, got, filepath.FromSlash(tt.local)) + } + } + if got := m.localPath("file:///c:/inetpub/wwwroot/x.php"); got != filepath.FromSlash("/home/me/win/x.php") { + t.Errorf("localPath(lower-case drive) = %q", got) + } + + // Local Windows paths, as dbgp on Windows sees them. + for _, tt := range []struct { + p, prefix, sep, rest string + ok bool + }{ + {`c:\project\src\a.php`, `C:\Project`, `\`, `src\a.php`, true}, + {`C:\PROJECT`, `C:\Project`, `\`, "", true}, + {`\\server\SHARE\a.php`, `\\Server\share`, `\`, "a.php", true}, + {`C:\Project2\a.php`, `C:\Project`, `\`, "", false}, + {"/home/Me/a.php", "/home/me", "/", "", false}, + } { + if rest, ok := cutPathPrefix(tt.p, tt.prefix, tt.sep, false); rest != tt.rest || ok != tt.ok { + t.Errorf("cutPathPrefix(%q, %q) = %q, %v; want %q, %v", tt.p, tt.prefix, rest, ok, tt.rest, tt.ok) + } + } +} + +// A mapping for a Windows directory replaces one for the same directory in +// another case. +func TestAddPathMappingWindowsCase(t *testing.T) { + srv := newTestServer(t, Config{}) + for _, local := range []string{`C:\Project`, `c:\project`} { + if err := srv.AddPathMapping(PathMapping{Local: local, Remote: "/app"}); err != nil { + t.Fatal(err) + } + } + if got := srv.PathMap(); len(got) != 1 || got[0].Local != `c:\project` { + t.Errorf("PathMap = %+v, want only the newer mapping", got) + } +} + +// On macOS and Windows, whose disks ignore case, local paths match a +// mapping in any case. Remote paths are still compared exactly, since PHP +// in a Linux container sees /var/www and /VAR/www as different. +func TestPathMapLocalCase(t *testing.T) { + for _, ignoreCase := range []bool{true, false} { + setLocalIgnoresCase(t, ignoreCase) + m := mustPathMap(t, PathMapping{Local: "/Users/me/Project", Remote: "/var/www/html"}) + + local := filepath.FromSlash("/users/ME/project/src/A.php") + want := filepath.ToSlash(local) // no mapping matches + if ignoreCase { + want = "/var/www/html/src/A.php" // the rest keeps its own case + } + if got := m.ToRemote(local); got != want { + t.Errorf("ignoreCase=%v: ToRemote(%q) = %q, want %q", ignoreCase, local, got, want) + } + // Whole components only. + other := filepath.FromSlash("/users/me/project2/a.php") + if got := m.ToRemote(other); got != filepath.ToSlash(other) { + t.Errorf("ignoreCase=%v: ToRemote(%q) = %q, want it unchanged", ignoreCase, other, got) + } + if got, want := m.ToLocal("/VAR/www/html/a.php"), filepath.FromSlash("/VAR/www/html/a.php"); got != want { + t.Errorf("ignoreCase=%v: ToLocal = %q, want %q", ignoreCase, got, want) + } + } +} + +// On macOS and Windows, a mapping for a local directory replaces one for +// the same directory in another case. +func TestAddPathMappingLocalCase(t *testing.T) { + for _, ignoreCase := range []bool{true, false} { + setLocalIgnoresCase(t, ignoreCase) + srv := newTestServer(t, Config{}) + for _, local := range []string{"/Users/me/Project", "/users/me/project"} { + if err := srv.AddPathMapping(PathMapping{Local: local, Remote: "/app"}); err != nil { + t.Fatal(err) + } + } + want := 2 + if ignoreCase { + want = 1 + } + if got := srv.PathMap(); len(got) != want || got[len(got)-1].Local != filepath.FromSlash("/users/me/project") { + t.Errorf("ignoreCase=%v: PathMap = %+v, want %d mappings, ending with the newer one", ignoreCase, got, want) + } + } +} + +func setLocalIgnoresCase(t *testing.T, v bool) { + old := localIgnoresCase + localIgnoresCase = v + t.Cleanup(func() { localIgnoresCase = old }) +} diff --git a/protocol.go b/protocol.go index f793dc2..d747ed3 100644 --- a/protocol.go +++ b/protocol.go @@ -11,49 +11,25 @@ import ( "path/filepath" "strings" "unicode" + + "golang.org/x/text/encoding/charmap" ) -// charsetReader handles non-UTF-8 XML encodings +// charsetReader handles non-UTF-8 XML encodings, decoding as it reads, so a +// large packet is not copied whole. func charsetReader(encoding string, input io.Reader) (io.Reader, error) { switch strings.ToLower(encoding) { case "utf-8", "us-ascii": return input, nil case "iso-8859-1", "latin1": - return decodeSingleByte(input, nil) + return charmap.ISO8859_1.NewDecoder().Reader(input), nil case "windows-1252": - return decodeSingleByte(input, windows1252Overrides) + return charmap.Windows1252.NewDecoder().Reader(input), nil default: return nil, fmt.Errorf("unsupported charset: %s", encoding) } } -func decodeSingleByte(input io.Reader, overrides map[byte]rune) (io.Reader, error) { - data, err := io.ReadAll(input) - if err != nil { - return nil, err - } - - runes := make([]rune, len(data)) - for i, b := range data { - if overrides != nil { - if mapped, ok := overrides[b]; ok { - runes[i] = mapped - continue - } - } - runes[i] = rune(b) - } - - return strings.NewReader(string(runes)), nil -} - -var windows1252Overrides = map[byte]rune{ - 0x80: '€', 0x82: '‚', 0x83: 'ƒ', 0x84: '„', 0x85: '…', 0x86: '†', 0x87: '‡', 0x88: 'ˆ', - 0x89: '‰', 0x8A: 'Š', 0x8B: '‹', 0x8C: 'Œ', 0x8E: 'Ž', 0x91: '‘', 0x92: '’', 0x93: '“', - 0x94: '”', 0x95: '•', 0x96: '–', 0x97: '—', 0x98: '˜', 0x99: '™', 0x9A: 'š', 0x9B: '›', - 0x9C: 'œ', 0x9E: 'ž', 0x9F: 'Ÿ', -} - // Status values const ( StatusStarting = "starting" diff --git a/protocol_test.go b/protocol_test.go index e727e11..6a35a8a 100644 --- a/protocol_test.go +++ b/protocol_test.go @@ -133,7 +133,7 @@ func TestParseResponseFields(t *testing.T) { } for name, want := range map[string]int{"run_break": lineAddBody, "step_over": lineAddReturn, "run_break_conditional": lineLoopBody} { file, line := parse(name).ParseMessage() - if file != "/app/basic.php" || line != want { + if file != filepath.FromSlash("/app/basic.php") || line != want { t.Errorf("%s: ParseMessage = %s:%d, want /app/basic.php:%d", name, file, line, want) } } @@ -205,7 +205,8 @@ func TestFormatStack(t *testing.T) { {Level: 0, Where: "add", Filename: "file:///app/basic.php", Lineno: 24}, {Level: 1, Where: "{main}", Filename: "file:///app/basic.php", Lineno: 36}, } - want := []string{"#0 add() at /app/basic.php:24", "#1 {main}() at /app/basic.php:36"} + file := filepath.FromSlash("/app/basic.php") + want := []string{"#0 add() at " + file + ":24", "#1 {main}() at " + file + ":36"} if got := FormatStack(frames); !reflect.DeepEqual(got, want) { t.Errorf("FormatStack = %q, want %q", got, want) } @@ -213,7 +214,7 @@ func TestFormatStack(t *testing.T) { func TestFormatStackDecodesURI(t *testing.T) { got := FormatStack([]StackFrame{{Where: "f", Filename: "file:///my%20app/a.php", Lineno: 1}}) - if want := "#0 f() at /my app/a.php:1"; got[0] != want { + if want := "#0 f() at " + filepath.FromSlash("/my app/a.php") + ":1"; got[0] != want { t.Errorf("FormatStack = %q, want %q", got[0], want) } } diff --git a/server.go b/server.go index 09149be..b62629d 100644 --- a/server.go +++ b/server.go @@ -12,6 +12,8 @@ import ( "strings" "sync" "time" + + "golang.org/x/sync/semaphore" ) // DefaultAddr is where PHP Debugger and Xdebug 3 connect by default: @@ -22,6 +24,10 @@ const DefaultAddr = "localhost:9003" // ErrServerClosed is returned by WaitForSession after Close. var ErrServerClosed = errors.New("server closed") +// ErrUnclaimed is returned by Claim for a session being detached because +// nobody took it within Config.ClaimTimeout. +var ErrUnclaimed = errors.New("session is being detached: nobody took it in time") + // StdoutMode selects what happens to a session's program output. type StdoutMode int @@ -51,9 +57,18 @@ type Config struct { // InitTimeout bounds how long a new connection may take to send its // init packet. Default: 10s. InitTimeout time.Duration + // CommandTimeout bounds how long a command may take to reach the engine + // and be answered; continuing the script is not bounded (see + // Session.Continue). Default: 30s. + CommandTimeout time.Duration // PathMap maps local directories to the engine's, for PHP running in a // container or on a remote server. Sessions take and return local paths. PathMap []PathMapping + // NoEval stops the server from running PHP code of its own in sessions: + // setup skips the eval that asks the engine whether it maps paths + // itself, so Session.EngineMapping reports nothing detected. Session + // methods that run code, such as EvalProperty, still do when called. + NoEval bool // ClaimTimeout, when set, detaches sessions that neither WaitForSession // nor Claim has taken within this time of connecting, so their scripts // run on without the debugger instead of waiting for someone to debug @@ -71,6 +86,15 @@ const ( // maxLiveSessions bounds sessions that have not ended. Beyond it, new // scripts are detached, so they run on without the debugger. maxLiveSessions = 16 + // setupTimeout bounds setting up a session after its init packet: + // features, output capture and breakpoints, a command each. A + // connection that takes longer is dropped, so one that stops answering + // frees its handshake slot. + setupTimeout = 30 * time.Second + // maxPacketsInFlight bounds the bytes of the packets a server's sessions + // are reading and parsing at once, which takes a few times as much + // memory. At least maxPacketBytes, so every packet fits. + maxPacketsInFlight = 2 * maxPacketBytes ) // Breakpoint is a breakpoint the server applies to every session, including @@ -109,6 +133,7 @@ type Server struct { warnings []string // addresses the last StartListening could not use closed bool handshakes map[net.Conn]struct{} + reserved int // live session slots taken by handshakes setting up nextID int sessions []*Session queue []*Session // not yet returned by WaitForSession @@ -118,6 +143,11 @@ type Server struct { // The connection limits; tests lower them. maxHandshakes, maxLiveSessions int + setupTimeout time.Duration + + // packetBudget holds maxPacketsInFlight bytes, which sessions take + // while they read and parse a packet. + packetBudget *semaphore.Weighted } // Listen creates a server and starts listening for engine connections. @@ -140,6 +170,9 @@ func NewServer(cfg Config) (*Server, error) { if cfg.InitTimeout <= 0 { cfg.InitTimeout = 10 * time.Second } + if cfg.CommandTimeout <= 0 { + cfg.CommandTimeout = defaultCommandTimeout + } paths, err := newPathMap(cfg.PathMap) if err != nil { return nil, err @@ -152,6 +185,8 @@ func NewServer(cfg Config) (*Server, error) { arrived: make(chan struct{}), maxHandshakes: maxHandshakes, maxLiveSessions: maxLiveSessions, + setupTimeout: setupTimeout, + packetBudget: semaphore.NewWeighted(maxPacketsInFlight), }, nil } @@ -179,7 +214,7 @@ func (s *Server) StartListening() error { var listeners []net.Listener var failures []string for _, h := range hosts { - listener, err := net.Listen("tcp", net.JoinHostPort(h, port)) + listener, err := listenTCP(net.JoinHostPort(h, port)) if err != nil { failures = append(failures, err.Error()) continue @@ -352,25 +387,34 @@ func (s *Server) handshake(conn net.Conn) { return } + // The slot is reserved until the session is added, so handshakes + // setting up at the same time cannot go past the limit together. s.mu.Lock() - if s.liveSessionsLocked() >= s.maxLiveSessions { + if s.liveSessionsLocked()+s.reserved >= s.maxLiveSessions { s.mu.Unlock() detachConn(conn) return } + s.reserved++ s.nextID++ id := s.nextID + setupTimeout := s.setupTimeout s.mu.Unlock() + // One deadline for the whole setup: once it passes, the session ends + // and its remaining commands fail at once. + _ = conn.SetDeadline(time.Now().Add(setupTimeout)) sess := newSession(id, s, conn, reader, init) sess.setup(s.cfg, s.pathMap()) // Nothing reports errors here: the first Continue reports rejected // breakpoints, and retries the others. _ = sess.syncBreakpoints(false) + _ = conn.SetDeadline(time.Time{}) s.mu.Lock() defer s.mu.Unlock() - if s.closed { + s.reserved-- + if s.closed || sess.ended() { _ = sess.Close() return } @@ -385,15 +429,34 @@ func (s *Server) handshake(conn net.Conn) { } // detachUnclaimed detaches sess if nobody has taken it, so its script runs -// on instead of waiting. +// on instead of waiting. Meanwhile Claim refuses it. func (s *Server) detachUnclaimed(sess *Session) { s.mu.Lock() unclaimed := !s.closed && s.unqueueLocked(sess) + sess.unclaimed = unclaimed s.mu.Unlock() - if unclaimed { - _ = sess.Detach() - _ = sess.Close() + if !unclaimed { + return } + _ = sess.Detach() + _ = sess.Close() + <-sess.Done() + s.mu.Lock() + sess.unclaimed = false + s.mu.Unlock() +} + +// reservePacket waits until the server's sessions are reading few enough +// packets to read one of length bytes, and returns the function that ends +// the read. A session without a server, in tests, does not wait. +func (s *Server) reservePacket(ctx context.Context, length int) (release func(), err error) { + if s == nil { + return func() {}, nil + } + if err := s.packetBudget.Acquire(ctx, int64(length)); err != nil { + return nil, err + } + return func() { s.packetBudget.Release(int64(length)) }, nil } // detachConn lets the script on conn run on without the debugger. @@ -421,7 +484,9 @@ func (sess *Session) setup(cfg Config, paths PathMap) { _ = sess.FeatureSet("notify_ok", "1") _ = sess.FeatureSet("resolved_breakpoints", "1") // Before cfg.Features, which may lower max_data below the probe's reply. - sess.detectEngineMapping(paths) + if !cfg.NoEval { + sess.detectEngineMapping(paths) + } names := make([]string, 0, len(cfg.Features)) for name := range cfg.Features { @@ -460,7 +525,7 @@ func (s *Server) PathMap() []PathMapping { } // AddPathMapping adds a mapping, or replaces the one for the same local -// directory. It applies to every session at once: paths from the engine are +// directory, in any case on Windows and macOS. It applies to every session at once: paths from the engine are // translated with it from now on, and breakpoints already set are set again // with their new paths, now in stopped sessions and on the next Continue in // running ones. The returned error lists sessions that rejected a breakpoint. @@ -473,7 +538,7 @@ func (s *Server) AddPathMapping(m PathMapping) error { s.pathsMu.Lock() paths := make(PathMap, 0, len(s.paths)+1) for _, old := range s.paths { - if old.Local != m.Local { + if !samePath(old.Local, m.Local, localIgnoresCase) { paths = append(paths, old) } } @@ -558,12 +623,18 @@ func (s *Server) WaitForSession(ctx context.Context) (*Session, error) { } // Claim takes sess as if WaitForSession had returned it, so WaitForSession -// does not return it and Config.ClaimTimeout no longer applies to it. It -// reports whether sess was still waiting to be taken. -func (s *Server) Claim(sess *Session) bool { +// does not return it and Config.ClaimTimeout no longer applies to it. +// Claiming a session already taken does nothing. It returns ErrUnclaimed +// while sess is being detached because nobody took it in time; once that +// is done, the session has ended. +func (s *Server) Claim(sess *Session) error { s.mu.Lock() defer s.mu.Unlock() - return s.unqueueLocked(sess) + if sess.unclaimed { + return ErrUnclaimed + } + s.unqueueLocked(sess) + return nil } // unqueueLocked removes sess from the sessions WaitForSession has not diff --git a/server_test.go b/server_test.go index 85f78a0..1a7a341 100644 --- a/server_test.go +++ b/server_test.go @@ -6,6 +6,7 @@ import ( "errors" "io" "net" + "slices" "sort" "strconv" "strings" @@ -127,6 +128,11 @@ func TestServerSessionSetup(t *testing.T) { "feature_set -n resolved_breakpoints -v 1", probeCommand, }}, + {"no eval", Config{NoEval: true}, []string{ + "feature_set -n notify_ok -v 1", + "feature_set -n resolved_breakpoints -v 1", + "stdout -c 1", + }}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -880,11 +886,11 @@ func TestServerDetachesUnclaimedSessions(t *testing.T) { waited := dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) waitSession(t, srv) claimed := dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) - if !srv.Claim(waitForSessionID(t, srv, 3)) { - t.Error("Claim of a new session = false") + if err := srv.Claim(waitForSessionID(t, srv, 3)); err != nil { + t.Errorf("Claim of a new session: %v", err) } - if srv.Claim(srv.Session(3)) || srv.Claim(srv.Session(2)) { - t.Error("Claim of a taken session = true") + if err := errors.Join(srv.Claim(srv.Session(3)), srv.Claim(srv.Session(2))); err != nil { + t.Errorf("Claim of a taken session: %v", err) } time.Sleep(3 * timeout) for _, e := range []*fakeEngine{waited, claimed} { @@ -912,3 +918,98 @@ func TestSessionRemoteAddr(t *testing.T) { t.Errorf("RemoteAddr() = %s, want 127.0.0.1", sess.RemoteAddr()) } } + +// A connection that sends its init packet and then stops answering is +// dropped once setup takes too long, which frees its handshake slot. +func TestServerDropsSilentSetup(t *testing.T) { + srv := newTestServer(t, Config{}) + setLimits(srv, 1, maxLiveSessions) + srv.mu.Lock() + srv.setupTimeout = 300 * time.Millisecond + srv.mu.Unlock() + + silent := dialFakeEngine(t, srv, fixture(t, "init"), map[string]fakeHandler{ + "feature_set": func(*fakeEngine, fakeCommand) []string { return nil }, + }) + if !silent.WaitClosed(3 * time.Second) { + t.Fatal("a connection that stopped answering during setup was kept") + } + // The slot is freed just after the connection is closed. + waitFor(t, func() bool { + srv.mu.Lock() + defer srv.mu.Unlock() + return len(srv.handshakes) == 0 + }) + dialFakeEngine(t, srv, fixture(t, "init"), standardHandlers()) + sess := waitSession(t, srv) + if got := srv.Sessions(); len(got) != 1 || got[0] != sess { + t.Errorf("sessions = %v, want only the one that answered", got) + } +} + +// Handshakes setting up at the same time cannot go past the live session +// limit together. +func TestServerLimitsConcurrentHandshakes(t *testing.T) { + srv := newTestServer(t, Config{}) + setLimits(srv, maxHandshakes, 1) + slow := func() map[string]fakeHandler { + handlers := standardHandlers() + handlers["feature_set"] = func(e *fakeEngine, cmd fakeCommand) []string { + time.Sleep(100 * time.Millisecond) + return reply("feature_set")(e, cmd) + } + return handlers + } + var engines []*fakeEngine + for range 3 { + engines = append(engines, dialFakeEngine(t, srv, fixture(t, "init"), slow())) + } + detached := func() int { + n := 0 + for _, e := range engines { + select { + case <-e.done: + n++ + default: + } + } + return n + } + waitFor(t, func() bool { return detached() == 2 }) + waitFor(t, func() bool { return len(srv.Sessions()) == 1 }) + time.Sleep(200 * time.Millisecond) + if n := len(srv.Sessions()); n != 1 || detached() != 2 { + t.Errorf("%d sessions and %d detached, want 1 and 2", n, detached()) + } +} + +// While a session nobody took is being detached, nobody can claim it, so it +// is not detached under them; once it has ended, claiming it is harmless. +func TestClaimWhileDetachingUnclaimed(t *testing.T) { + answer := make(chan struct{}) + var once sync.Once + answerDetach := func() { once.Do(func() { close(answer) }) } + defer answerDetach() + handlers := standardHandlers() + handlers["detach"] = func(e *fakeEngine, cmd fakeCommand) []string { + <-answer + return reply("detach")(e, cmd) + } + srv := newTestServer(t, Config{ClaimTimeout: 100 * time.Millisecond}) + e := dialFakeEngine(t, srv, fixture(t, "init"), handlers) + sess := waitForSessionID(t, srv, 1) + waitFor(t, func() bool { + return slices.ContainsFunc(e.Received(), func(c fakeCommand) bool { return c.Name == "detach" }) + }) + + if err := srv.Claim(sess); !errors.Is(err, ErrUnclaimed) { + t.Errorf("Claim while detaching = %v, want ErrUnclaimed", err) + } + answerDetach() + select { + case <-sess.Done(): + case <-time.After(3 * time.Second): + t.Fatal("the unclaimed session did not end") + } + waitFor(t, func() bool { return srv.Claim(sess) == nil }) +} diff --git a/session.go b/session.go index 8ab4564..90b9b30 100644 --- a/session.go +++ b/session.go @@ -15,11 +15,11 @@ import ( "sync" "sync/atomic" "time" + "unicode/utf8" ) -// commandTimeout bounds how long a command waits for its response. -// Continuation commands are not bounded by it: see Session.Continue. -const commandTimeout = 30 * time.Second +// defaultCommandTimeout is Config.CommandTimeout's default. +const defaultCommandTimeout = 30 * time.Second // Continuation commands resume the script, which then runs until it stops // again: at a breakpoint, after the step, or at the end of the script. @@ -33,10 +33,21 @@ const ( // Output and notification history kept per session; older entries are // dropped first. const ( - maxOutputBytes = 1 << 20 - maxNotifications = 1000 + maxOutputBytes = 1 << 20 + maxNotifications = 1000 + maxNotificationBytes = 1 << 20 + // maxMessageBytes bounds the text kept of one notification's message, + // such as a PHP warning: real ones, even with a stack trace, are + // shorter. + maxMessageBytes = 8 << 10 ) +// MaxValueBytes is the most of a value GetProperty and GetPropertyValue +// ask the engine for, whatever PropertyOptions.MaxData says: the engine +// sends it in one packet, base64-encoded, which must stay under the packet +// size limit. +const MaxValueBytes = 20 << 20 + // maxEndedSessions is how many ended sessions a Server keeps; older ones // are forgotten. const maxEndedSessions = 50 @@ -70,6 +81,13 @@ type Session struct { conn net.Conn init *InitPacket closed chan struct{} // closed when the read loop exits + // connCtx is cancelled when conn is closed. + connCtx context.Context + // cmdTimeout is Config.CommandTimeout; see commandTimeout. + cmdTimeout time.Duration + // unclaimed is set while the session is being detached because nobody + // took it in time. Guarded by server.mu. + unclaimed bool // cmdSlot serializes commands: at most one is in flight, so an error // response without a transaction_id belongs to it. @@ -97,6 +115,7 @@ type Session struct { outputBase int // stream offset of output[0] notes []Notification notesBase int // index of notes[0] among all notifications + notesBytes int // size of notes, see Notification.size setupErr error warnings []string engineMap EngineMapping @@ -184,10 +203,12 @@ type pendingCommand struct { // newSession starts reading responses for an engine connection whose init // packet has been read. func newSession(id int, server *Server, conn net.Conn, reader *bufio.Reader, init *InitPacket) *Session { + ctx, cancel := context.WithCancel(context.Background()) s := &Session{ id: id, server: server, - conn: conn, + conn: &sessionConn{Conn: conn, cancel: cancel}, + connCtx: ctx, init: init, closed: make(chan struct{}), cmdSlot: make(chan struct{}, 1), @@ -196,10 +217,25 @@ func newSession(id int, server *Server, conn net.Conn, reader *bufio.Reader, ini state: State{Status: StatusStarting}, changed: make(chan struct{}), } + if server != nil { + s.cmdTimeout = server.cfg.CommandTimeout + } go s.readLoop(reader) return s } +// sessionConn is a session's connection, whose Close also cancels the +// context the session reads with, so a read waiting for its turn ends too. +type sessionConn struct { + net.Conn + cancel context.CancelFunc +} + +func (c *sessionConn) Close() error { + c.cancel() + return c.Conn.Close() +} + // ID identifies the session within its server. func (s *Session) ID() int { return s.id } @@ -282,8 +318,9 @@ func (s *Session) Notifications(from int) ([]Notification, int) { // must not crash dbgp or make it allocate more than the data that arrives. const ( // maxPacketBytes bounds every packet. Responses can be large, such as a - // long string from property_value, but not this large. - maxPacketBytes = 256 << 20 + // long string from property_value (see MaxValueBytes), but not larger. + // A larger packet ends the session. + maxPacketBytes = 32 << 20 // maxInitPacketBytes bounds the init packet, which a connection sends // before it is known to be an engine. Real ones are well under 1 KB. maxInitPacketBytes = 64 << 10 @@ -298,7 +335,12 @@ func readPacket(reader *bufio.Reader, limit int) ([]byte, error) { if err != nil { return nil, err } + return readPacketData(reader, length) +} +// readPacketData reads the length bytes of a packet's data and the NUL +// that ends it. +func readPacketData(reader *bufio.Reader, length int) ([]byte, error) { // Grow the buffer as the data arrives rather than trusting the length. var data bytes.Buffer data.Grow(min(length, packetChunk)) @@ -350,27 +392,44 @@ func readPacketLength(reader *bufio.Reader, limit int) (int, error) { func (s *Session) readLoop(reader *bufio.Reader) { defer s.finish() for { - data, err := readPacket(reader, maxPacketBytes) + length, err := readPacketLength(reader, maxPacketBytes) if err != nil { return } - kind, err := packetKind(data) + // Reading and parsing a packet takes a few times its size: the + // server bounds how much its sessions read at once. + release, err := s.server.reservePacket(s.connCtx, length) if err != nil { - continue + return } - switch kind { - case "response": - if resp, err := ParseResponse(data); err == nil { - s.dispatch(resp) - } - case "stream": - if _, text, err := ParseStream(data); err == nil { - s.appendOutput(text) - } - case "notify": - if n, err := ParseNotification(data); err == nil { - s.addNotification(*n) - } + data, err := readPacketData(reader, length) + if err != nil { + release() + return + } + s.handlePacket(data) + release() + } +} + +// handlePacket parses a packet from the engine and acts on it. +func (s *Session) handlePacket(data []byte) { + kind, err := packetKind(data) + if err != nil { + return + } + switch kind { + case "response": + if resp, err := ParseResponse(data); err == nil { + s.dispatch(resp) + } + case "stream": + if _, text, err := ParseStream(data); err == nil { + s.appendOutput(text) + } + case "notify": + if n, err := ParseNotification(data); err == nil { + s.addNotification(*n) } } } @@ -450,19 +509,56 @@ func (s *Session) appendOutput(text string) { func (s *Session) addNotification(n Notification) { if n.Message != nil { n.Message.Filename = s.pathMap().localPath(n.Message.Filename) + n.Message.Text = cutUTF8(n.Message.Text, maxMessageBytes) } if n.Breakpoint != nil && n.Breakpoint.Filename != "" { n.Breakpoint.Filename = s.pathMap().localPath(n.Breakpoint.Filename) } + size := n.size() + if size > maxNotificationBytes/16 { + return // only a malicious engine sends one this large + } s.mu.Lock() defer s.mu.Unlock() s.notes = append(s.notes, n) - if over := len(s.notes) - maxNotifications; over > 0 { + s.notesBytes += size + over := 0 + for len(s.notes)-over > maxNotifications || s.notesBytes > maxNotificationBytes { + s.notesBytes -= s.notes[over].size() + over++ + } + if over > 0 { s.notes = append([]Notification(nil), s.notes[over:]...) s.notesBase += over } } +// size approximates the memory a notification takes: its strings, and a +// little for the rest. +func (n Notification) size() int { + size := 64 + len(n.Name) + if m := n.Message; m != nil { + size += len(m.Filename) + len(m.Exception) + len(m.Type) + len(m.Code) + len(m.Text) + } + if b := n.Breakpoint; b != nil { + size += len(b.Type) + len(b.Filename) + len(b.Function) + len(b.State) + len(b.Resolved) + + len(b.Exception) + len(b.HitCondition) + len(b.Expression) + } + return size +} + +// cutUTF8 returns at most the first n bytes of s, ending on a whole +// character. +func cutUTF8(s string, n int) string { + if len(s) <= n { + return s + } + for n > 0 && !utf8.RuneStart(s[n]) { + n-- + } + return s[:n] +} + // formatCommand builds a DBGp command line: // // name -a value ... -i id [-- base64(data)] @@ -534,7 +630,7 @@ func (s *Session) checkCanSendLocked() error { // sendCommand sends a command and waits for its response. Commands are // serialized, so this also waits for any command already in flight. func (s *Session) sendCommand(name string, args []string, data []byte) (*Response, error) { - timeout := time.NewTimer(commandTimeout) + timeout := time.NewTimer(s.commandTimeout()) defer timeout.Stop() if err := s.acquire(name, timeout.C); err != nil { return nil, err @@ -556,9 +652,9 @@ func (s *Session) sendCommand(name string, args []string, data []byte) (*Respons s.pending = p s.mu.Unlock() - if _, err := s.conn.Write([]byte(line + "\x00")); err != nil { + if err := s.writeCommand(line); err != nil { s.clearPending(p) - return nil, fmt.Errorf("send command: %w", err) + return nil, err } select { @@ -637,7 +733,7 @@ func (s *Session) Continue(ctx context.Context, command string, wait time.Durati // startContinuation sends a continuation command without waiting for the // script to stop; the response arrives when it does. func (s *Session) startContinuation(command string) error { - timeout := time.NewTimer(commandTimeout) + timeout := time.NewTimer(s.commandTimeout()) defer timeout.Stop() if err := s.acquire(command, timeout.C); err != nil { return err @@ -662,18 +758,42 @@ func (s *Session) startContinuation(command string) error { s.setStateLocked(State{Status: StatusRunning, Reason: "ok"}) s.mu.Unlock() - if _, err := s.conn.Write([]byte(line + "\x00")); err != nil { + if err := s.writeCommand(line); err != nil { s.mu.Lock() if s.contID == id { s.contID = 0 s.setStateLocked(prev) } s.mu.Unlock() + return err + } + return nil +} + +// writeCommand sends a command line to the engine. A peer that stops +// reading cannot block it for longer than the command timeout. A command +// cut short would garble the ones after it, so then the connection ends. +func (s *Session) writeCommand(line string) error { + _ = s.conn.SetWriteDeadline(time.Now().Add(s.commandTimeout())) + _, err := s.conn.Write([]byte(line + "\x00")) + _ = s.conn.SetWriteDeadline(time.Time{}) + if err != nil { + _ = s.conn.Close() return fmt.Errorf("send command: %w", err) } return nil } +// commandTimeout bounds how long a command waits to be sent and for its +// response. Continuation commands wait for their response unbounded: see +// Session.Continue. +func (s *Session) commandTimeout() time.Duration { + if s.cmdTimeout > 0 { + return s.cmdTimeout + } + return defaultCommandTimeout +} + // Wait waits up to wait for a running script to stop and returns the // session state, which has Status running if it has not stopped yet. func (s *Session) Wait(ctx context.Context, wait time.Duration) (State, error) { @@ -921,7 +1041,7 @@ func (o PropertyOptions) args(fullname string) []string { args = append(args, "-p", strconv.Itoa(o.Page)) } if o.MaxData > 0 { - args = append(args, "-m", strconv.Itoa(o.MaxData)) + args = append(args, "-m", strconv.Itoa(min(o.MaxData, MaxValueBytes))) } return args } diff --git a/session_test.go b/session_test.go index 857436e..4d2a10d 100644 --- a/session_test.go +++ b/session_test.go @@ -5,13 +5,17 @@ import ( "context" "encoding/base64" "errors" + "fmt" "io" + "path/filepath" "reflect" + "runtime" "strconv" "strings" "sync" "testing" "time" + "unicode/utf8" ) func TestSessionInit(t *testing.T) { @@ -317,7 +321,7 @@ func TestResponseDecoding(t *testing.T) { if want := []string{"line", "conditional", "call", "exception"}; !reflect.DeepEqual(types, want) { t.Fatalf("types = %v, want %v", types, want) } - if bps[0].ID != 42420001 || bps[0].Filename != "/app/basic.php" || bps[0].Lineno != 24 || bps[0].State != "enabled" { + if bps[0].ID != 42420001 || bps[0].Filename != filepath.FromSlash("/app/basic.php") || bps[0].Lineno != 24 || bps[0].State != "enabled" { t.Errorf("line breakpoint = %+v", bps[0]) } if bps[2].State != "disabled" { @@ -330,8 +334,8 @@ func TestResponseDecoding(t *testing.T) { t.Fatal(err) } want := []StackFrame{ - {Level: 0, Type: "file", Filename: "/app/basic.php", Lineno: 24, Where: "add"}, - {Level: 1, Type: "file", Filename: "/app/basic.php", Lineno: 36, Where: "{main}"}, + {Level: 0, Type: "file", Filename: filepath.FromSlash("/app/basic.php"), Lineno: 24, Where: "add"}, + {Level: 1, Type: "file", Filename: filepath.FromSlash("/app/basic.php"), Lineno: 36, Where: "{main}"}, } if !reflect.DeepEqual(stack, want) { t.Errorf("stack = %+v, want %+v", stack, want) @@ -720,7 +724,7 @@ func TestContinueStopsAtBreak(t *testing.T) { if err != nil { t.Fatal(err) } - want := State{Status: StatusBreak, Reason: "ok", File: "/app/basic.php", Line: lineAddBody} + want := State{Status: StatusBreak, Reason: "ok", File: filepath.FromSlash("/app/basic.php"), Line: lineAddBody} if st != want { t.Errorf("State = %+v, want %+v", st, want) } @@ -942,7 +946,7 @@ func TestNotificationsAreCollected(t *testing.T) { if next != 2 || notes[0].Name != "error" || notes[1].Name != "breakpoint_resolved" { t.Fatalf("Notifications = %+v, %d", notes, next) } - want := Message{Filename: "/app/basic.php", Lineno: lineWarning, Type: "Warning", Text: "Undefined variable $undefinedVariable"} + want := Message{Filename: filepath.FromSlash("/app/basic.php"), Lineno: lineWarning, Type: "Warning", Text: "Undefined variable $undefinedVariable"} if *notes[0].Message != want { t.Errorf("error notification = %+v, want %+v", *notes[0].Message, want) } @@ -1096,3 +1100,144 @@ func TestSetPropertyFailure(t *testing.T) { t.Error("SetProperty succeeded although the engine reported failure") } } + +// Sessions take turns reading packets when together they would read more +// than the server allows at once, and a session waiting for its turn still +// ends when it is closed. +func TestPacketBudget(t *testing.T) { + sess, e := startFakeSession(t, standardHandlers()) + srv := sess.server + stream := `` + + base64.StdEncoding.EncodeToString([]byte("hi")) + `` + + // With the whole budget taken, the packet waits. + if err := srv.packetBudget.Acquire(context.Background(), maxPacketsInFlight); err != nil { + t.Fatal(err) + } + e.Send(stream) + time.Sleep(100 * time.Millisecond) + if text, _, _ := sess.Output(0); text != "" { + t.Fatalf("output %q read without budget", text) + } + srv.packetBudget.Release(maxPacketsInFlight) + waitFor(t, func() bool { text, _, _ := sess.Output(0); return text == "hi" }) + + // Closing the session ends the wait. + if err := srv.packetBudget.Acquire(context.Background(), maxPacketsInFlight); err != nil { + t.Fatal(err) + } + defer srv.packetBudget.Release(maxPacketsInFlight) + e.Send(stream) + time.Sleep(100 * time.Millisecond) + _ = sess.Close() + select { + case <-sess.Done(): + case <-time.After(3 * time.Second): + t.Fatal("session waiting for budget did not end when closed") + } +} + +// Notifications are bounded in bytes too, and each message in length. +func TestNotificationBytesAreBounded(t *testing.T) { + s := &Session{} + long := strings.Repeat("é", maxMessageBytes) // two bytes each + for i := 0; i < maxNotifications; i++ { + s.addNotification(Notification{Name: strconv.Itoa(i), Message: &Message{Text: long}}) + } + notes, next := s.Notifications(0) + if next != maxNotifications { + t.Errorf("next = %d, want %d", next, maxNotifications) + } + text := notes[len(notes)-1].Message.Text + if len(text) > maxMessageBytes || len(text) < maxMessageBytes-1 || !utf8.ValidString(text) { + t.Errorf("message cut to %d bytes, valid UTF-8 %v; want about %d", len(text), utf8.ValidString(text), maxMessageBytes) + } + total := 0 + for _, n := range notes { + total += n.size() + } + if total > maxNotificationBytes || s.notesBytes != total || len(notes) == maxNotifications { + t.Errorf("%d notes of %d bytes (counted %d), want at most %d bytes, oldest dropped", + len(notes), total, s.notesBytes, maxNotificationBytes) + } + + // One far too large to be real is dropped. + s.addNotification(Notification{Name: "huge", Message: &Message{Filename: strings.Repeat("x", maxNotificationBytes)}}) + if _, after := s.Notifications(0); after != next { + t.Errorf("a huge notification was kept") + } +} + +func TestPropertyMaxDataIsCapped(t *testing.T) { + args := PropertyOptions{MaxData: 1 << 30}.args("$big") + if got := args[len(args)-1]; got != strconv.Itoa(MaxValueBytes) { + t.Errorf("args = %v, want -m %d", args, MaxValueBytes) + } +} + +// A command the engine does not answer fails after Config.CommandTimeout. +func TestCommandTimeout(t *testing.T) { + srv := newTestServer(t, Config{CommandTimeout: 200 * time.Millisecond}) + handlers := standardHandlers() + handlers["stack_get"] = func(*fakeEngine, fakeCommand) []string { return nil } + dialFakeEngine(t, srv, fixture(t, "init"), handlers) + sess := waitSession(t, srv) + start := time.Now() + if _, err := sess.GetStack(); err == nil || !strings.Contains(err.Error(), "timeout waiting for response") { + t.Errorf("GetStack = %v, want a timeout", err) + } + if took := time.Since(start); took > 2*time.Second { + t.Errorf("GetStack took %v, want about 200ms", took) + } +} + +// An engine that stops reading cannot block a command for longer than the +// command timeout: the write fails, and the session ends, since the rest of +// the command would garble what follows. +func TestWriteDeadline(t *testing.T) { + if runtime.GOOS == "windows" { + // Windows takes in a 40 MB command whole, even with a small receive + // buffer, so the write does not block; the command timeout applies. + t.Skip("writes to an engine that stopped reading do not block on Windows") + } + srv := newTestServer(t, Config{CommandTimeout: 200 * time.Millisecond}) + conn := dialServer(t, srv) + init := fixture(t, "init") + fmt.Fprintf(conn, "%d\x00%s\x00", len(init), init) + // Answer the setup commands, up to output capture, the last one. + reader := bufio.NewReader(conn) + for { + line, err := reader.ReadString(0) + if err != nil { + t.Fatal(err) + } + cmd := parseFakeCommand(strings.TrimSuffix(line, "\x00")) + resp := fmt.Sprintf(``, cmd.Name, cmd.Args["i"]) + fmt.Fprintf(conn, "%d\x00%s\x00", len(resp), resp) + if cmd.Name == "stdout" { + break + } + } + sess := waitSession(t, srv) + + // From now on the engine reads nothing: a command larger than the + // socket buffers cannot be written whole. + done := make(chan error, 1) + go func() { + _, err := sess.EvalProperty(strings.Repeat("1+", 16<<20)+"1", 0) + done <- err + }() + select { + case err := <-done: + if err == nil || !strings.Contains(err.Error(), "send command") { + t.Errorf("EvalProperty = %v, want the write to fail", err) + } + case <-time.After(3 * time.Second): + t.Fatal("a command to an engine that stopped reading was still being written after 3s") + } + select { + case <-sess.Done(): + case <-time.After(3 * time.Second): + t.Error("the session did not end after a write failed") + } +}