From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17466 invoked by alias); 9 Oct 2008 03:30:26 -0000 Received: (qmail 17440 invoked by uid 22791); 9 Oct 2008 03:30:24 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 09 Oct 2008 03:29:46 +0000 Received: (qmail 28679 invoked from network); 9 Oct 2008 03:29:44 -0000 Received: from unknown (HELO orlando.local) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 9 Oct 2008 03:29:44 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: [commit] remote: make the whole connection setup exception safe Date: Thu, 09 Oct 2008 03:30:00 -0000 User-Agent: KMail/1.9.9 MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_pqX7IVIYcZmcL6L" Message-Id: <200810090429.45346.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-10/txt/msg00277.txt.bz2 --Boundary-00=_pqX7IVIYcZmcL6L Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Content-Disposition: inline Content-length: 2320 (gdb) tar rem | asdfasdfa Remote debugging using | asdfasdfa sh: asdfasdfa: not found Remote communication error: Resource temporarily unavailable. (gdb) p a ../../src/gdb/thread.c:529: internal-error: is_thread_state: Assertion `tp' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) Ok, doesn't look good, and when I wrote this patch, the symptoms weren't so bad, but this is really undefined behaviour ... The big picture: We got ourselves a half-backed target pushed on the stack. Not very digestible. The issue is that we have this remote_start_remote function that is wrapped in catch_exceptions, where the initial comunication with the target should be done. At its call site, we have this comment: remote_open_1: " /* Start the remote connection. If error() or QUIT, discard this target (we'd otherwise be in an inconsistent state) and then propogate the error on up the exception chain. This ensures that the caller doesn't stumble along blindly assuming that the function succeeded. " But, over time, several possible calls to putpkt/getpkt/error made its way to remote_open_1, making it possible to leave a half-backed target pushed when an exception is thrown. So, the patch looks bigger than it really is. I'm just mostly moving things around (but keeping the same sequences). (The initial acknowleding of any pending ack, which should be the first thing sent to the stub used to be in remote_start_remote, until I moved it out, with the noack mode changes, because I had noticed that it was no longer the first thing being sent.) Daniel then found out that this made an existing race easier to trigger. If the connection is closed while inside remote_start_remote, readchar calls target_mourn_inferior followed by throwing an error. This exception is then caught in the safe net of remote_open_1 mentioned about, that pops the target. Problem is, remote_mourn_1 had already unpushed the target. This meant that the second pop could pop the dummy target --- not something that we should be doing every day. The following commands would crash, because the target_stack would be borked. Tested against a local x86-64-unknown-linux-gnu gdbserver and checked in. -- Pedro Alves --Boundary-00=_pqX7IVIYcZmcL6L Content-Type: text/x-diff; charset="utf-8"; name="remote_setup_excp_safe.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="remote_setup_excp_safe.diff" Content-length: 7224 2008-10-09 Pedro Alves Daniel Jacobowitz * remote.c (remote_open_1): Move acknowledging any pending ack, querying supported features, activating noack mode, finding the target description, enabling extended remote, and checking remote symbols from here ... (remote_start_remote): ... to here. (remote_open_1): Don't pop the target if it is already gone. * target.c (unpush_target): Check for the dummy target. --- gdb/remote.c | 115 +++++++++++++++++++++++++++++++---------------------------- gdb/target.c | 4 ++ 2 files changed, 65 insertions(+), 54 deletions(-) Index: src/gdb/remote.c =================================================================== --- src.orig/gdb/remote.c 2008-10-09 03:20:08.000000000 +0100 +++ src/gdb/remote.c 2008-10-09 03:26:18.000000000 +0100 @@ -211,6 +211,10 @@ static void show_remote_protocol_packet_ static char *write_ptid (char *buf, const char *endbuf, ptid_t ptid); static ptid_t read_ptid (char *buf, char **obuf); +static void remote_query_supported (void); + +static void remote_check_symbols (struct objfile *objfile); + void _initialize_remote (void); /* For "remote". */ @@ -2375,12 +2379,50 @@ struct start_remote_args static void remote_start_remote (struct ui_out *uiout, void *opaque) { - struct remote_state *rs = get_remote_state (); struct start_remote_args *args = opaque; + struct remote_state *rs = get_remote_state (); + struct packet_config *noack_config; char *wait_status = NULL; immediate_quit++; /* Allow user to interrupt it. */ + /* Ack any packet which the remote side has already sent. */ + serial_write (remote_desc, "+", 1); + + /* The first packet we send to the target is the optional "supported + packets" request. If the target can answer this, it will tell us + which later probes to skip. */ + remote_query_supported (); + + /* Next, we possibly activate noack mode. + + If the QStartNoAckMode packet configuration is set to AUTO, + enable noack mode if the stub reported a wish for it with + qSupported. + + If set to TRUE, then enable noack mode even if the stub didn't + report it in qSupported. If the stub doesn't reply OK, the + session ends with an error. + + If FALSE, then don't activate noack mode, regardless of what the + stub claimed should be the default with qSupported. */ + + noack_config = &remote_protocol_packets[PACKET_QStartNoAckMode]; + + if (noack_config->detect == AUTO_BOOLEAN_TRUE + || (noack_config->detect == AUTO_BOOLEAN_AUTO + && noack_config->support == PACKET_ENABLE)) + { + putpkt ("QStartNoAckMode"); + getpkt (&rs->buf, &rs->buf_size, 0); + if (packet_ok (rs->buf, noack_config) == PACKET_OK) + rs->noack_mode = 1; + } + + /* Next, if the target can specify a description, read it. We do + this before anything involving memory or registers. */ + target_find_description (); + /* Check whether the target is running now. */ putpkt ("?"); getpkt (&rs->buf, &rs->buf_size, 0); @@ -2439,6 +2481,20 @@ remote_start_remote (struct ui_out *uiou immediate_quit--; start_remote (args->from_tty); /* Initialize gdb process mechanisms. */ + + if (args->extended_p) + { + /* Tell the remote that we are using the extended protocol. */ + putpkt ("!"); + getpkt (&rs->buf, &rs->buf_size, 0); + } + + /* If we connected to a live target, do some additional setup. */ + if (target_has_execution) + { + if (exec_bfd) /* No use without an exec file. */ + remote_check_symbols (symfile_objfile); + } } /* Open a connection to a remote debugger. @@ -2788,7 +2844,6 @@ static void remote_open_1 (char *name, int from_tty, struct target_ops *target, int extended_p) { struct remote_state *rs = get_remote_state (); - struct packet_config *noack_config; if (name == 0) error (_("To open a remote debug connection, you need to specify what\n" @@ -2883,43 +2938,6 @@ remote_open_1 (char *name, int from_tty, use_threadinfo_query = 1; use_threadextra_query = 1; - /* Ack any packet which the remote side has already sent. */ - serial_write (remote_desc, "+", 1); - - /* The first packet we send to the target is the optional "supported - packets" request. If the target can answer this, it will tell us - which later probes to skip. */ - remote_query_supported (); - - /* Next, we possibly activate noack mode. - - If the QStartNoAckMode packet configuration is set to AUTO, - enable noack mode if the stub reported a wish for it with - qSupported. - - If set to TRUE, then enable noack mode even if the stub didn't - report it in qSupported. If the stub doesn't reply OK, the - session ends with an error. - - If FALSE, then don't activate noack mode, regardless of what the - stub claimed should be the default with qSupported. */ - - noack_config = &remote_protocol_packets[PACKET_QStartNoAckMode]; - - if (noack_config->detect == AUTO_BOOLEAN_TRUE - || (noack_config->detect == AUTO_BOOLEAN_AUTO - && noack_config->support == PACKET_ENABLE)) - { - putpkt ("QStartNoAckMode"); - getpkt (&rs->buf, &rs->buf_size, 0); - if (packet_ok (rs->buf, noack_config) == PACKET_OK) - rs->noack_mode = 1; - } - - /* Next, if the target can specify a description, read it. We do - this before anything involving memory or registers. */ - target_find_description (); - if (target_async_permitted) { /* With this target we start out by owning the terminal. */ @@ -2964,7 +2982,10 @@ remote_open_1 (char *name, int from_tty, ex = catch_exception (uiout, remote_start_remote, &args, RETURN_MASK_ALL); if (ex.reason < 0) { - pop_target (); + /* Pop the partially set up target - unless something else did + already before throwing the exception. */ + if (remote_desc != NULL) + pop_target (); if (target_async_permitted) wait_forever_enabled_p = 1; throw_exception (ex); @@ -2973,20 +2994,6 @@ remote_open_1 (char *name, int from_tty, if (target_async_permitted) wait_forever_enabled_p = 1; - - if (extended_p) - { - /* Tell the remote that we are using the extended protocol. */ - putpkt ("!"); - getpkt (&rs->buf, &rs->buf_size, 0); - } - - /* If we connected to a live target, do some additional setup. */ - if (target_has_execution) - { - if (exec_bfd) /* No use without an exec file. */ - remote_check_symbols (symfile_objfile); - } } /* This takes a program previously attached to and detaches it. After Index: src/gdb/target.c =================================================================== --- src.orig/gdb/target.c 2008-10-09 03:21:50.000000000 +0100 +++ src/gdb/target.c 2008-10-09 03:26:18.000000000 +0100 @@ -746,6 +746,10 @@ unpush_target (struct target_ops *t) struct target_ops **cur; struct target_ops *tmp; + if (t->to_stratum == dummy_stratum) + internal_error (__FILE__, __LINE__, + "Attempt to unpush the dummy target"); + /* Look for the specified target. Note that we assume that a target can only occur once in the target stack. */ --Boundary-00=_pqX7IVIYcZmcL6L--