From 2827145f1e887ac90d048752e21f591367bc1b5b Mon Sep 17 00:00:00 2001 From: Alessandro Arzilli Date: Wed, 8 Jun 2022 19:56:50 +0200 Subject: [PATCH] proc/gdbserver: ignore spurious stop packet from debugserver (#3021) When we send an interrupt request to debugserver we, sometimes, get one extra spurious stop packet back. This stop packet gets interpreted as a response to a qThreadStopInfo request we make causing the protocol to become desynchronized for a while, until eventually some kind of error appears. Here's an example of this problem, distilled from issue #3013: 1 <- $vCont;c#a8 2 <- interrupt 3 -> $T05thread:12efb47;threads:12efb47,12efb8d,12efb8e,12efb8f,12efb90,12efb91;thread-pcs:10abe83,7ff81b20e2be,7ff81b20e3ea,... 4 <- $qThreadStopInfo12efb8e#28 5 -> $T05thread:12efb47;threads:12efb47,12efb8d,12efb8e,12efb8f,12efb90,12efb91;thread-pcs:10abe83,7ff81b20e2be,7ff81b20e3ea,... 6 <- $qThreadStopInfo12efb8f#29 7 -> $T00thread:12efb8e;threads:12efb47,12efb8d,12efb8e,12efb8f,12efb90,12efb91;thread-pcs:10abe83,7ff81b20e2be,7ff81b20e3ea,... 8 <- $qThreadStopInfo12efb90#f4 9 -> $T00thread:12efb8f;threads:12efb47,12efb8d,12efb8e,12efb8f,12efb90,12efb91;thread-pcs:10abe83,7ff81b20e2be,7ff81b20e3ea,... 10 <- $qThreadStopInfo12efb91#f5 11 -> $T00thread:12efb90;threads:12efb47,12efb8d,12efb8e,12efb8f,12efb90,12efb91;thread-pcs:10abe83,7ff81b20e2be,7ff81b20e3ea,... 12 <- $qThreadStopInfo12efb47#f6 13 -> $T00thread:12efb91;threads:12efb47,12efb8d,12efb8e,12efb8f,12efb90,12efb91;thread-pcs:10abe83,7ff81b20e2be,7ff81b20e3ea,... 14 <- $qThreadStopInfo12efb8d#27 15 -> $T05thread:12efb47;threads:12efb47,12efb8d,12efb8e,12efb8f,12efb90,12efb91;thread-pcs:10abe83,7ff81b20e2be,7ff81b20e3ea,... 16 <- $p0;thread:12efb8e;#f5 17 -> $T00thread:12efb8d;threads:12efb47,12efb8d,12efb8e,12efb8f,12efb90,12efb91;thread-pcs:10abe83,7ff81b20e2be,7ff81b20e3ea,... response (3) is interpreted as the response to the vCont request at (1). We then make a qThreadStopInfo request (4) and receive a stop packet in response (5). Packet (5) is interpreted as the response to (4) but it actually isn't, note how the thread ID is different, packet (5) is actually a spurious stop packet sent by debug server. From response (5) onward the protocol is desynchronized, none of the response we process are actually the response to the preceding request. This eventually causes a failure at packet (17) which debugserver sent as the response to request (14) but we interpret as the response to (16). Fixes #3013 --- pkg/proc/gdbserial/gdbserver.go | 3 ++ pkg/proc/gdbserial/gdbserver_conn.go | 43 ++++++++++++++++++++++++---- 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/pkg/proc/gdbserial/gdbserver.go b/pkg/proc/gdbserial/gdbserver.go index 7c0ca13f..38be0be8 100644 --- a/pkg/proc/gdbserial/gdbserver.go +++ b/pkg/proc/gdbserial/gdbserver.go @@ -849,6 +849,9 @@ continueLoop: // For stubs that support qThreadStopInfo updateThreadList will // find out the reason why each thread stopped. + // NOTE: because debugserver will sometimes send two stop packets after a + // continue it is important that this is the very first thing we do after + // resume(). See comment in threadStopInfo for an explanation. p.updateThreadList(&tu) trapthread = p.findThreadByStrID(threadID) diff --git a/pkg/proc/gdbserial/gdbserver_conn.go b/pkg/proc/gdbserial/gdbserver_conn.go index c5227474..66489857 100644 --- a/pkg/proc/gdbserial/gdbserver_conn.go +++ b/pkg/proc/gdbserial/gdbserver_conn.go @@ -256,7 +256,8 @@ func setRegFound(regFound map[string]bool, name string) { // readTargetXml reads target.xml file from stub using qXfer:features:read, // then parses it requesting any additional files. // The schema of target.xml is described by: -// https://github.com/bminor/binutils-gdb/blob/61baf725eca99af2569262d10aca03dcde2698f6/gdb/features/gdb-target.dtd +// +// https://github.com/bminor/binutils-gdb/blob/61baf725eca99af2569262d10aca03dcde2698f6/gdb/features/gdb-target.dtd func (conn *gdbConn) readTargetXml(regFound map[string]bool) (err error) { conn.regsInfo, err = conn.readAnnex("target.xml") if err != nil { @@ -715,9 +716,10 @@ type stopPacket struct { // Mach exception codes used to decode metype/medata keys in stop packets (necessary to support watchpoints with debugserver). // See: -// https://opensource.apple.com/source/xnu/xnu-4570.1.46/osfmk/mach/exception_types.h.auto.html -// https://opensource.apple.com/source/xnu/xnu-4570.1.46/osfmk/mach/i386/exception.h.auto.html -// https://opensource.apple.com/source/xnu/xnu-4570.1.46/osfmk/mach/arm/exception.h.auto.html +// +// https://opensource.apple.com/source/xnu/xnu-4570.1.46/osfmk/mach/exception_types.h.auto.html +// https://opensource.apple.com/source/xnu/xnu-4570.1.46/osfmk/mach/i386/exception.h.auto.html +// https://opensource.apple.com/source/xnu/xnu-4570.1.46/osfmk/mach/arm/exception.h.auto.html const ( _EXC_BREAKPOINT = 6 // mach exception type for hardware breakpoints _EXC_I386_SGL = 1 // mach exception code for single step on x86, for some reason this is also used for watchpoints @@ -1095,6 +1097,36 @@ func (conn *gdbConn) threadStopInfo(threadID string) (sp stopPacket, err error) if err != nil { return stopPacket{}, err } + if sp.threadID != threadID { + // When we send a ^C (manual stop request) and the process is close to + // stopping anyway, sometimes, debugserver will send back two stop + // packets. We need to ignore this spurious stop packet. Because the first + // thing we do after the stop is updateThreadList, which calls this + // function, this is relatively painless. We simply need to check that the + // stop packet we receive is for the thread we requested, if it isn't we + // can assume it is the spurious extra stop packet and simply ignore it. + // An example of a problematic interaction is in the commit message for + // this change. + // See https://github.com/go-delve/delve/issues/3013. + + conn.conn.SetReadDeadline(time.Now().Add(10 * time.Millisecond)) + resp, err = conn.recv(conn.outbuf.Bytes(), "thread stop info", false) + conn.conn.SetReadDeadline(time.Time{}) + if err != nil { + if neterr, isneterr := err.(net.Error); isneterr && neterr.Timeout() { + return stopPacket{}, fmt.Errorf("qThreadStopInfo mismatch, requested %s got %s", sp.threadID, threadID) + } + return stopPacket{}, err + } + _, sp, err = conn.parseStopPacket(resp, "", nil) + if err != nil { + return stopPacket{}, err + } + if sp.threadID != threadID { + return stopPacket{}, fmt.Errorf("qThreadStopInfo mismatch, requested %s got %s", sp.threadID, threadID) + } + } + return sp, nil } @@ -1246,7 +1278,8 @@ func (conn *gdbConn) memoryRegionInfo(addr uint64) (*memoryRegionInfo, error) { // exec executes a message to the stub and reads a response. // The details of the wire protocol are described here: -// https://sourceware.org/gdb/onlinedocs/gdb/Overview.html#Overview +// +// https://sourceware.org/gdb/onlinedocs/gdb/Overview.html#Overview func (conn *gdbConn) exec(cmd []byte, context string) ([]byte, error) { if err := conn.send(cmd); err != nil { return nil, err