This is an automated email from Gerrit.
Jan Matyas (mat...@codasip.com) just uploaded a new patch set to Gerrit, which
you can find at http://openocd.zylin.com/6309
-- gerrit
commit 131420ac649a0027e65d65eb17adeeb96dc9013a
Author: Jan Matyas <mat...@codasip.com>
Date: Tue Jun 8 08:38:03 2021 +0200
server: Fix socket listen(), do not listen for gdb prematurely
Avoid the situation when OpenOCD allows incoming clients
to connect (listen()) but is not yet ready to service them
(accept() + I/O).
In current OpenOCD, this issue affected the gdb service:
GDB clients were permitted to connect even before OpenOCD
was able to handle them - gdb succeeded to connect
but got no response.
Steps to reproduce:
1) Prepare a .cfg file:
jtag newtap ....
target create ....
init
puts "Simulating long action, e.g. load_image ..."
after 60000
puts "Long action completed".
2) Launch OpenOCD
3) Connect gdb to OpenOCD:
(gdb) target extended-remote localhost:3333
4) Observe the gdb output & the TCP connection state (wireshark/tcpdump):
4a) OpenOCD without this patch:
(gdb) target extended-remote localhost:3333
Remote debugging using localhost:3333
Ignoring packet error, continuing...
warning: unrecognized item "timeout" in "qSupported" response
Ignoring packet error, continuing...
Remote replied unexpectedly to 'vMustReplyEmpty': timeout
... GDB connects successfully but gets no response to the usual packets
4b) OpenOCD with this fix:
(gdb) target extended-remote localhost:3333
localhost:3333: Connection timed out.
... Correct behavior: Connection between GDB and OpenOCD cannot be
established until OpenOCD is fully initialized and ready to talk to
the GDB client.
Change-Id: I00966f59b1b5b63f1cdadbc513c23e0202573706
Signed-off-by: Jan Matyas <mat...@codasip.com>
diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c
index d0586d1..fe63321 100644
--- a/src/server/gdb_server.c
+++ b/src/server/gdb_server.c
@@ -3521,7 +3521,7 @@ static int gdb_target_start(struct target *target, const
char *port)
if (NULL == gdb_service)
return -ENOMEM;
- LOG_INFO("starting gdb server for %s on %s", target_name(target), port);
+ LOG_INFO("starting gdb server for %s on port %s", target_name(target),
port);
gdb_service->target = target;
gdb_service->core[0] = -1;
diff --git a/src/server/server.c b/src/server/server.c
index 3072663..0a3ee17 100644
--- a/src/server/server.c
+++ b/src/server/server.c
@@ -46,6 +46,7 @@
#endif
static struct service *services;
+static bool already_listening;
enum shutdown_reason {
CONTINUE_MAIN_LOOP, /* stay in main event loop */
@@ -205,6 +206,43 @@ static void free_service(struct service *c)
free(c);
}
+static int service_tcp_listen(struct service *service)
+{
+ assert(service->type == CONNECTION_TCP);
+
+#ifndef _WIN32
+ int segsize = 65536;
+ setsockopt(service->fd, IPPROTO_TCP, TCP_MAXSEG, &segsize,
sizeof(int));
+#endif
+ int window_size = 128 * 1024;
+
+ /* These setsockopt()s must happen before the listen() */
+ setsockopt(service->fd, SOL_SOCKET, SO_SNDBUF,
+ (char *)&window_size, sizeof(window_size));
+ setsockopt(service->fd, SOL_SOCKET, SO_RCVBUF,
+ (char *)&window_size, sizeof(window_size));
+
+ /* Start listening */
+ if (listen(service->fd, 1) == -1) {
+ LOG_ERROR("couldn't listen for %s connections: %s",
service->name, strerror(errno));
+ close_socket(service->fd);
+ }
+
+ /* Inform the user */
+ struct sockaddr_in addr_in;
+ addr_in.sin_port = 0;
+ socklen_t addr_in_size = sizeof(addr_in);
+ if (getsockname(service->fd, (struct sockaddr *)&addr_in,
&addr_in_size) == 0) {
+ LOG_INFO("Listening on port %hu for %s connections",
+ ntohs(addr_in.sin_port),
+ service->name);
+ } else {
+ LOG_WARNING("getsockname() failed on %s socket", service->name);
+ }
+
+ return ERROR_OK;
+}
+
int add_service(char *name,
const char *port,
int max_connections,
@@ -229,6 +267,7 @@ int add_service(char *name,
c->connection_closed = connection_closed_handler;
c->priv = priv;
c->next = NULL;
+
long portnumber;
if (strcmp(c->port, "pipe") == 0)
c->type = CONNECTION_STDINOUT;
@@ -284,32 +323,14 @@ int add_service(char *name,
return ERROR_FAIL;
}
-#ifndef _WIN32
- int segsize = 65536;
- setsockopt(c->fd, IPPROTO_TCP, TCP_MAXSEG, &segsize,
sizeof(int));
-#endif
- int window_size = 128 * 1024;
-
- /* These setsockopt()s must happen before the listen() */
-
- setsockopt(c->fd, SOL_SOCKET, SO_SNDBUF,
- (char *)&window_size, sizeof(window_size));
- setsockopt(c->fd, SOL_SOCKET, SO_RCVBUF,
- (char *)&window_size, sizeof(window_size));
-
- if (listen(c->fd, 1) == -1) {
- LOG_ERROR("couldn't listen on socket: %s",
strerror(errno));
- close_socket(c->fd);
- free_service(c);
- return ERROR_FAIL;
+ /* Only start listening if the server is already running and
ready to speak to the clients. */
+ if (already_listening) {
+ if (service_tcp_listen(c) != ERROR_OK) {
+ free_service(c);
+ return ERROR_FAIL;
+ }
}
- struct sockaddr_in addr_in;
- addr_in.sin_port = 0;
- socklen_t addr_in_size = sizeof(addr_in);
- if (getsockname(c->fd, (struct sockaddr *)&addr_in,
&addr_in_size) == 0)
- LOG_INFO("Listening on port %hu for %s connections",
- ntohs(addr_in.sin_port), name);
} else if (c->type == CONNECTION_STDINOUT) {
c->fd = fileno(stdin);
@@ -350,6 +371,25 @@ int add_service(char *name,
return ERROR_OK;
}
+static int start_listen(void)
+{
+ /* Safety: This is to be called just once when activating server. */
+ assert(!already_listening);
+
+ /* Listen for all TCP services that are already registered. */
+ for (struct service *s = services; s; s = s->next) {
+ if (s->type == CONNECTION_TCP) {
+ if (service_tcp_listen(s) != ERROR_OK)
+ return ERROR_FAIL;
+ }
+ }
+
+ /* Services registered later will listen immediately. */
+ already_listening = true;
+
+ return ERROR_OK;
+}
+
static void remove_connections(struct service *service)
{
struct connection *connection;
@@ -442,6 +482,10 @@ int server_loop(struct command_context *command_context)
LOG_ERROR("couldn't set SIGPIPE to SIG_IGN");
#endif
+ /* Start listening for all services */
+ if (start_listen() != ERROR_OK)
+ shutdown_openocd = SHUTDOWN_WITH_ERROR_CODE;
+
while (shutdown_openocd == CONTINUE_MAIN_LOOP) {
/* monitor sockets for activity */
fd_max = 0;
--
