From 9eb900c4485e5a508a15696b8c4916381c09fdfb Mon Sep 17 00:00:00 2001 From: sg Date: Thu, 19 Mar 2015 21:20:29 +0100 Subject: [PATCH] fixed race conditions in assigning netconn->last_err (fixed bugs #38121 and #37676) --- CHANGELOG | 4 ++++ src/api/api_lib.c | 15 +++------------ src/api/api_msg.c | 16 +++++++++++----- src/include/lwip/api.h | 4 ++-- src/include/lwip/tcpip.h | 4 ++-- 5 files changed, 22 insertions(+), 21 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 697597ca..b1189e4d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -216,6 +216,10 @@ HISTORY ++ Bugfixes: + 2015-03-19: Simon Goldschmidt + * api.h, tcpip.h, api_lib.c, api_msg.c: fixed race conditions in assigning + netconn->last_err (fixed bugs #38121 and #37676) + 2015-03-09: Simon Goldschmidt * ip4.c: fixed the IPv4 part of bug #43904 (ip_route() must detect linkup status) diff --git a/src/api/api_lib.c b/src/api/api_lib.c index d476ab2a..aada0b6e 100644 --- a/src/api/api_lib.c +++ b/src/api/api_lib.c @@ -181,7 +181,6 @@ netconn_getaddr(struct netconn *conn, ip_addr_t *addr, u16_t *port, u8_t local) #endif /* LWIP_MPU_COMPATIBLE */ API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; } @@ -215,7 +214,6 @@ netconn_bind(struct netconn *conn, const ip_addr_t *addr, u16_t port) TCPIP_APIMSG(&API_MSG_VAR_REF(msg), lwip_netconn_do_bind, err); API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; } @@ -247,7 +245,6 @@ netconn_connect(struct netconn *conn, const ip_addr_t *addr, u16_t port) TCPIP_APIMSG(&API_MSG_VAR_REF(msg), lwip_netconn_do_connect, err); API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; } @@ -270,7 +267,6 @@ netconn_disconnect(struct netconn *conn) TCPIP_APIMSG(&API_MSG_VAR_REF(msg), lwip_netconn_do_disconnect, err); API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; } @@ -302,7 +298,6 @@ netconn_listen_with_backlog(struct netconn *conn, u8_t backlog) TCPIP_APIMSG(&API_MSG_VAR_REF(msg), lwip_netconn_do_listen, err); API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; #else /* LWIP_TCP */ LWIP_UNUSED_ARG(conn); @@ -343,7 +338,6 @@ netconn_accept(struct netconn *conn, struct netconn **new_conn) #if LWIP_SO_RCVTIMEO if (sys_arch_mbox_fetch(&conn->acceptmbox, (void **)&newconn, conn->recv_timeout) == SYS_ARCH_TIMEOUT) { - NETCONN_SET_SAFE_ERR(conn, ERR_TIMEOUT); return ERR_TIMEOUT; } #else @@ -354,6 +348,9 @@ netconn_accept(struct netconn *conn, struct netconn **new_conn) if (newconn == NULL) { /* connection has been aborted */ + /* in this special case, we set the netconn error from application thread, as + on a ready-to-accept listening netconn, there should not be anything running + in tcpip_thread */ NETCONN_SET_SAFE_ERR(conn, ERR_ABRT); return ERR_ABRT; } @@ -422,7 +419,6 @@ netconn_recv_data(struct netconn *conn, void **new_buf) #if LWIP_SO_RCVTIMEO if (sys_arch_mbox_fetch(&conn->recvmbox, &buf, conn->recv_timeout) == SYS_ARCH_TIMEOUT) { - NETCONN_SET_SAFE_ERR(conn, ERR_TIMEOUT); return ERR_TIMEOUT; } #else @@ -533,7 +529,6 @@ netconn_recv(struct netconn *conn, struct netbuf **new_buf) buf = (struct netbuf *)memp_malloc(MEMP_NETBUF); if (buf == NULL) { - NETCONN_SET_SAFE_ERR(conn, ERR_MEM); return ERR_MEM; } @@ -639,7 +634,6 @@ netconn_send(struct netconn *conn, struct netbuf *buf) TCPIP_APIMSG(&API_MSG_VAR_REF(msg), lwip_netconn_do_send, err); API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; } @@ -711,7 +705,6 @@ netconn_write_partly(struct netconn *conn, const void *dataptr, size_t size, } API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; } @@ -745,7 +738,6 @@ netconn_close_shutdown(struct netconn *conn, u8_t how) TCPIP_APIMSG(&API_MSG_VAR_REF(msg), lwip_netconn_do_close, err); API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; } @@ -812,7 +804,6 @@ netconn_join_leave_group(struct netconn *conn, TCPIP_APIMSG(&API_MSG_VAR_REF(msg), lwip_netconn_do_join_leave_group, err); API_MSG_VAR_FREE(msg); - NETCONN_SET_SAFE_ERR(conn, err); return err; } #endif /* LWIP_IGMP || (LWIP_IPV6 && LWIP_IPV6_MLD) */ diff --git a/src/api/api_msg.c b/src/api/api_msg.c index 79306cd4..96c4462e 100644 --- a/src/api/api_msg.c +++ b/src/api/api_msg.c @@ -251,7 +251,9 @@ recv_tcp(void *arg, struct tcp_pcb *pcb, struct pbuf *p, err_t err) (data is already ACKed) */ /* don't overwrite fatal errors! */ - NETCONN_SET_SAFE_ERR(conn, err); + if (err != ERR_OK) { + NETCONN_SET_SAFE_ERR(conn, err); + } if (p != NULL) { len = p->tot_len; @@ -416,6 +418,7 @@ err_tcp(void *arg, err_t err) LWIP_ASSERT("inavlid op_completed_sem", op_completed_sem != SYS_SEM_NULL); conn->current_msg = NULL; /* wake up the waiting task */ + NETCONN_SET_SAFE_ERR(conn, err); sys_sem_signal(op_completed_sem); } } else { @@ -934,6 +937,7 @@ lwip_netconn_do_close_internal(struct netconn *conn WRITE_DELAYED_PARAM) API_EVENT(conn, NETCONN_EVT_SENDPLUS, 0); } } + NETCONN_SET_SAFE_ERR(conn, err); #if LWIP_TCPIP_CORE_LOCKING if (delayed) #endif @@ -988,6 +992,7 @@ lwip_netconn_do_delconn(struct api_msg_msg *msg) msg->conn->current_msg = NULL; msg->conn->write_offset = 0; msg->conn->state = NETCONN_NONE; + NETCONN_SET_SAFE_ERR(msg->conn, ERR_CLSD); sys_sem_signal(op_completed_sem); } } @@ -1140,9 +1145,7 @@ lwip_netconn_do_connected(void *arg, struct tcp_pcb *pcb, err_t err) (!was_blocking && op_completed_sem == NULL)); conn->current_msg = NULL; conn->state = NETCONN_NONE; - if (!was_blocking) { - NETCONN_SET_SAFE_ERR(conn, ERR_OK); - } + NETCONN_SET_SAFE_ERR(conn, ERR_OK); API_EVENT(conn, NETCONN_EVT_SENDPLUS, 0); if (was_blocking) { @@ -1539,6 +1542,7 @@ err_mem: conn->current_msg->err = err; conn->current_msg = NULL; conn->state = NETCONN_NONE; + NETCONN_SET_SAFE_ERR(conn, err); #if LWIP_TCPIP_CORE_LOCKING if (delayed) #endif @@ -1704,6 +1708,7 @@ lwip_netconn_do_close(struct api_msg_msg *msg) msg->conn->current_msg = NULL; msg->conn->write_offset = 0; msg->conn->state = NETCONN_NONE; + NETCONN_SET_SAFE_ERR(msg->conn, ERR_CLSD); sys_sem_signal(op_completed_sem); } else { LWIP_ASSERT("msg->msg.sd.shut == NETCONN_SHUT_RD", msg->msg.sd.shut == NETCONN_SHUT_RD); @@ -1742,7 +1747,8 @@ lwip_netconn_do_close(struct api_msg_msg *msg) { msg->err = ERR_CONN; } - sys_sem_signal(LWIP_API_MSG_SEM(msg)); + NETCONN_SET_SAFE_ERR(msg->conn, msg->err); + TCPIP_APIMSG_ACK(msg); } #if LWIP_IGMP || (LWIP_IPV6 && LWIP_IPV6_MLD) diff --git a/src/include/lwip/api.h b/src/include/lwip/api.h index e5058f56..a00c4dce 100644 --- a/src/include/lwip/api.h +++ b/src/include/lwip/api.h @@ -235,14 +235,14 @@ struct netconn { } /** Set conn->last_err to err but don't overwrite fatal errors */ -#define NETCONN_SET_SAFE_ERR(conn, err) do { \ +#define NETCONN_SET_SAFE_ERR(conn, err) do { if ((conn) != NULL) { \ SYS_ARCH_DECL_PROTECT(lev); \ SYS_ARCH_PROTECT(lev); \ if (!ERR_IS_FATAL((conn)->last_err)) { \ (conn)->last_err = err; \ } \ SYS_ARCH_UNPROTECT(lev); \ -} while(0); +}} while(0); /* Network connection functions: */ #define netconn_new(t) netconn_new_with_proto_and_callback(t, 0, NULL) diff --git a/src/include/lwip/tcpip.h b/src/include/lwip/tcpip.h index cadccd96..0fd93322 100644 --- a/src/include/lwip/tcpip.h +++ b/src/include/lwip/tcpip.h @@ -81,7 +81,7 @@ extern sys_mutex_t lock_tcpip_core; TCPIP_APIMSG_NOERR(m,f); \ (e) = (m)->msg.err; \ } while(0) -#define TCPIP_APIMSG_ACK(m) +#define TCPIP_APIMSG_ACK(m) NETCONN_SET_SAFE_ERR((m)->conn, (m)->err) #define TCPIP_NETIFAPI(m) tcpip_netifapi_lock(m) #define TCPIP_NETIFAPI_ACK(m) #define TCPIP_PPPAPI(m) tcpip_pppapi_lock(m) @@ -91,7 +91,7 @@ extern sys_mutex_t lock_tcpip_core; #define UNLOCK_TCPIP_CORE() #define TCPIP_APIMSG_NOERR(m,f) do { (m)->function = f; tcpip_apimsg(m); } while(0) #define TCPIP_APIMSG(m,f,e) do { (m)->function = f; (e) = tcpip_apimsg(m); } while(0) -#define TCPIP_APIMSG_ACK(m) sys_sem_signal(LWIP_API_MSG_SEM(m)) +#define TCPIP_APIMSG_ACK(m) do { NETCONN_SET_SAFE_ERR((m)->conn, (m)->err); sys_sem_signal(LWIP_API_MSG_SEM(m)); } while(0) #define TCPIP_NETIFAPI(m) tcpip_netifapi(m) #define TCPIP_NETIFAPI_ACK(m) sys_sem_signal(&m->sem) #define TCPIP_PPPAPI(m) tcpip_pppapi(m)