Hi Joel,
I raised the bug & created the patchset, attached here as well. It's
against the STABLE-2_0_0 branch.
BR,
David
On 08/09/17 20:32, Joel Cunningham wrote:
David,
Would you mind opening a bug report at
https://savannah.nongnu.org/bugs/?func=additem&group=lwip and
attaching the patches so we can do the review there?
Also, it would be preferable to have the changes in a git patchset
that can be applied to the repo (easier to review than looking at
individual patch files)
Thanks
Joel
On Sep 8, 2017, at 9:50 AM, David Lockyer
<david.lock...@chronos.co.uk <mailto:david.lock...@chronos.co.uk>> wrote:
So something like attached?
I removed my raise/reset macro's.
It appears to work for me, I added a new private pool, not sure if
this was what you intended or not.
Let me know what you think.
BR
David
On 07/09/17 19:40, goldsi...@gmx.de wrote:
David Lockyer wrote:
Okay, thank you for the suggestion. Just to be clear are you
suggesting modifying lwip_select() to allocate select_cb from a
pool & free prior to return?
Via a define, like Joel wrote, yes. This might need a new memp pool
though...
We don't have an abstraction for your priviledge raising macro yet,
and I'm not sure it's the best solution for everyone using this
mode, either.
I will have to investigate the speed impact of this, as I have
MEM_LIBC_MALLOC and MAEP_MEM_MALLOC both defined as 1.
Well, you have this pool allocation at some other places already
when socket threads communicate with the tcpip thread for
asynchronous things.
Having those 2 defines combined with a possibly slow libc malloc()
(heap) is probabably not the fastest solution, anyway.
The best thing to get this fixed would be
Simon
______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud
service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________
_______________________________________________
lwip-users mailing list
lwip-users@nongnu.org
https://lists.nongnu.org/mailman/listinfo/lwip-users
______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________
<src_api_sockets_c.patch><src_include_lwip_opt_h.patch><src_include_lwip_priv_memp_std_h.patch><src_include_lwip_sockets_h.patch>_______________________________________________
lwip-users mailing list
lwip-users@nongnu.org <mailto:lwip-users@nongnu.org>
https://lists.nongnu.org/mailman/listinfo/lwip-users
______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________
_______________________________________________
lwip-users mailing list
lwip-users@nongnu.org
https://lists.nongnu.org/mailman/listinfo/lwip-users
______________________________________________________________________
This email has been scanned by the Symantec Email Security.cloud service.
For more information please visit http://www.symanteccloud.com
______________________________________________________________________
>From 2b1ee0de3af88b5f85c451906a8854fc7b9c21ae Mon Sep 17 00:00:00 2001
From: David Lockyer <david.lock...@chronos.co.uk>
Date: Mon, 11 Sep 2017 10:29:41 +0100
Subject: [PATCH] Fixed a memory access fault in lwip_select(), when using MPU.
---
src/api/sockets.c | 85 +++++++++++++++++-----------------------
src/include/lwip/opt.h | 8 ++++
src/include/lwip/priv/memp_std.h | 4 ++
src/include/lwip/sockets.h | 27 +++++++++++++
4 files changed, 74 insertions(+), 50 deletions(-)
diff --git a/src/api/sockets.c b/src/api/sockets.c
index b763248..83812b1 100644
--- a/src/api/sockets.c
+++ b/src/api/sockets.c
@@ -50,7 +50,6 @@
#include "lwip/sockets.h"
#include "lwip/api.h"
-#include "lwip/sys.h"
#include "lwip/igmp.h"
#include "lwip/inet.h"
#include "lwip/tcp.h"
@@ -77,6 +76,13 @@
#define LWIP_NETCONN 0
#endif
+#define API_SELECT_CB_VAR_REF(name) API_VAR_REF(name)
+#define API_SELECT_CB_VAR_DECLARE(name) API_VAR_DECLARE(struct lwip_select_cb, name)
+#define API_SELECT_CB_VAR_ALLOC(name) API_VAR_ALLOC(struct lwip_select_cb, MEMP_SELECT_CB, name, ERR_MEM)
+#define API_SELECT_CB_VAR_ALLOC_RETURN_NULL(name) API_VAR_ALLOC(struct lwip_select_cb, MEMP_SELECT_CB, name, NULL)
+#define API_SELECT_CB_VAR_FREE(name) API_VAR_FREE(MEMP_SELECT_CB, name)
+
+
#if LWIP_IPV4
#define IP4ADDR_PORT_TO_SOCKADDR(sin, ipaddr, port) do { \
(sin)->sin_len = sizeof(struct sockaddr_in); \
@@ -218,32 +224,6 @@ struct lwip_sock {
SELWAIT_T select_waiting;
};
-#if LWIP_NETCONN_SEM_PER_THREAD
-#define SELECT_SEM_T sys_sem_t*
-#define SELECT_SEM_PTR(sem) (sem)
-#else /* LWIP_NETCONN_SEM_PER_THREAD */
-#define SELECT_SEM_T sys_sem_t
-#define SELECT_SEM_PTR(sem) (&(sem))
-#endif /* LWIP_NETCONN_SEM_PER_THREAD */
-
-/** Description for a task waiting in select */
-struct lwip_select_cb {
- /** Pointer to the next waiting task */
- struct lwip_select_cb *next;
- /** Pointer to the previous waiting task */
- struct lwip_select_cb *prev;
- /** readset passed to select */
- fd_set *readset;
- /** writeset passed to select */
- fd_set *writeset;
- /** unimplemented: exceptset passed to select */
- fd_set *exceptset;
- /** don't signal the same semaphore twice: set to 1 when signalled */
- int sem_signalled;
- /** semaphore to wake up a task waiting for select */
- SELECT_SEM_T sem;
-};
-
/** A struct sockaddr replacement that has the same alignment as sockaddr_in/
* sockaddr_in6 if instantiated.
*/
@@ -1375,7 +1355,7 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset,
int nready;
fd_set lreadset, lwriteset, lexceptset;
u32_t msectimeout;
- struct lwip_select_cb select_cb;
+ API_SELECT_CB_VAR_DECLARE(select_cb);
int i;
int maxfdp2;
#if LWIP_NETCONN_SEM_PER_THREAD
@@ -1383,6 +1363,8 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset,
#endif
SYS_ARCH_DECL_PROTECT(lev);
+ API_SELECT_CB_VAR_ALLOC(select_cb);
+
LWIP_DEBUGF(SOCKETS_DEBUG, ("lwip_select(%d, %p, %p, %p, tvsec=%"S32_F" tvusec=%"S32_F")\n",
maxfdp1, (void *)readset, (void *) writeset, (void *) exceptset,
timeout ? (s32_t)timeout->tv_sec : (s32_t)-1,
@@ -1406,18 +1388,19 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset,
list is only valid while we are in this function, so it's ok
to use local variables. */
- select_cb.next = NULL;
- select_cb.prev = NULL;
- select_cb.readset = readset;
- select_cb.writeset = writeset;
- select_cb.exceptset = exceptset;
- select_cb.sem_signalled = 0;
+ API_SELECT_CB_VAR_REF(select_cb).next = NULL;
+ API_SELECT_CB_VAR_REF(select_cb).prev = NULL;
+ API_SELECT_CB_VAR_REF(select_cb).readset = readset;
+ API_SELECT_CB_VAR_REF(select_cb).writeset = writeset;
+ API_SELECT_CB_VAR_REF(select_cb).exceptset = exceptset;
+ API_SELECT_CB_VAR_REF(select_cb).sem_signalled = 0;
#if LWIP_NETCONN_SEM_PER_THREAD
- select_cb.sem = LWIP_NETCONN_THREAD_SEM_GET();
+ API_SELECT_CB_VAR_REF(select_cb).sem = LWIP_NETCONN_THREAD_SEM_GET();
#else /* LWIP_NETCONN_SEM_PER_THREAD */
- if (sys_sem_new(&select_cb.sem, 0) != ERR_OK) {
+ if (sys_sem_new(&API_SELECT_CB_VAR_REF(select_cb).sem, 0) != ERR_OK) {
/* failed to create semaphore */
set_errno(ENOMEM);
+ API_SELECT_CB_VAR_FREE(select_cb);
return -1;
}
#endif /* LWIP_NETCONN_SEM_PER_THREAD */
@@ -1426,11 +1409,11 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset,
SYS_ARCH_PROTECT(lev);
/* Put this select_cb on top of list */
- select_cb.next = select_cb_list;
+ API_SELECT_CB_VAR_REF(select_cb).next = select_cb_list;
if (select_cb_list != NULL) {
- select_cb_list->prev = &select_cb;
+ select_cb_list->prev = &API_SELECT_CB_VAR_REF(select_cb);
}
- select_cb_list = &select_cb;
+ select_cb_list = &API_SELECT_CB_VAR_REF(select_cb);
/* Increasing this counter tells event_callback that the list has changed. */
select_cb_ctr++;
@@ -1477,7 +1460,7 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset,
}
}
- waitres = sys_arch_sem_wait(SELECT_SEM_PTR(select_cb.sem), msectimeout);
+ waitres = sys_arch_sem_wait(SELECT_SEM_PTR(API_SELECT_CB_VAR_REF(select_cb).sem), msectimeout);
#if LWIP_NETCONN_SEM_PER_THREAD
waited = 1;
#endif
@@ -1507,32 +1490,33 @@ lwip_select(int maxfdp1, fd_set *readset, fd_set *writeset, fd_set *exceptset,
}
/* Take us off the list */
SYS_ARCH_PROTECT(lev);
- if (select_cb.next != NULL) {
- select_cb.next->prev = select_cb.prev;
+ if (API_SELECT_CB_VAR_REF(select_cb).next != NULL) {
+ API_SELECT_CB_VAR_REF(select_cb).next->prev = API_SELECT_CB_VAR_REF(select_cb).prev;
}
- if (select_cb_list == &select_cb) {
- LWIP_ASSERT("select_cb.prev == NULL", select_cb.prev == NULL);
- select_cb_list = select_cb.next;
+ if (select_cb_list == &API_SELECT_CB_VAR_REF(select_cb)) {
+ LWIP_ASSERT("select_cb.prev == NULL", API_SELECT_CB_VAR_REF(select_cb).prev == NULL);
+ select_cb_list = API_SELECT_CB_VAR_REF(select_cb).next;
} else {
- LWIP_ASSERT("select_cb.prev != NULL", select_cb.prev != NULL);
- select_cb.prev->next = select_cb.next;
+ LWIP_ASSERT("select_cb.prev != NULL", API_SELECT_CB_VAR_REF(select_cb).prev != NULL);
+ API_SELECT_CB_VAR_REF(select_cb).prev->next = API_SELECT_CB_VAR_REF(select_cb).next;
}
/* Increasing this counter tells event_callback that the list has changed. */
select_cb_ctr++;
SYS_ARCH_UNPROTECT(lev);
#if LWIP_NETCONN_SEM_PER_THREAD
- if (select_cb.sem_signalled && (!waited || (waitres == SYS_ARCH_TIMEOUT))) {
+ if (API_SELECT_CB_VAR_REF(select_cb).sem_signalled && (!waited || (waitres == SYS_ARCH_TIMEOUT))) {
/* don't leave the thread-local semaphore signalled */
- sys_arch_sem_wait(select_cb.sem, 1);
+ sys_arch_sem_wait(API_SELECT_CB_VAR_REF(select_cb).sem, 1);
}
#else /* LWIP_NETCONN_SEM_PER_THREAD */
- sys_sem_free(&select_cb.sem);
+ sys_sem_free(&API_SELECT_CB_VAR_REF(select_cb).sem);
#endif /* LWIP_NETCONN_SEM_PER_THREAD */
if (nready < 0) {
/* This happens when a socket got closed while waiting */
set_errno(EBADF);
+ API_SELECT_CB_VAR_FREE(select_cb);
return -1;
}
@@ -1560,6 +1544,7 @@ return_copy_fdsets:
if (exceptset) {
*exceptset = lexceptset;
}
+ API_SELECT_CB_VAR_FREE(select_cb);
return nready;
}
diff --git a/src/include/lwip/opt.h b/src/include/lwip/opt.h
index fd459af..01a09f9 100644
--- a/src/include/lwip/opt.h
+++ b/src/include/lwip/opt.h
@@ -455,6 +455,14 @@
#endif
/**
+ * MEMP_NUM_SELECT_CB: the number of struct lwip_select_cb.
+ * (only needed if you use the sequential API, like api_lib.c)
+ */
+#if !defined MEMP_NUM_SELECT_CB || defined __DOXYGEN__
+#define MEMP_NUM_SELECT_CB 4
+#endif
+
+/**
* MEMP_NUM_TCPIP_MSG_API: the number of struct tcpip_msg, which are used
* for callback/timeout API communication.
* (only needed if you use tcpip.c)
diff --git a/src/include/lwip/priv/memp_std.h b/src/include/lwip/priv/memp_std.h
index ce9fd50..144d965 100644
--- a/src/include/lwip/priv/memp_std.h
+++ b/src/include/lwip/priv/memp_std.h
@@ -64,6 +64,10 @@ LWIP_MEMPOOL(NETBUF, MEMP_NUM_NETBUF, sizeof(struct netbuf),
LWIP_MEMPOOL(NETCONN, MEMP_NUM_NETCONN, sizeof(struct netconn), "NETCONN")
#endif /* LWIP_NETCONN || LWIP_SOCKET */
+#if LWIP_SOCKET
+LWIP_MEMPOOL(SELECT_CB, MEMP_NUM_SELECT_CB, sizeof(struct lwip_select_cb), "SELECT_CB")
+#endif /* LWIP_SOCKET */
+
#if NO_SYS==0
LWIP_MEMPOOL(TCPIP_MSG_API, MEMP_NUM_TCPIP_MSG_API, sizeof(struct tcpip_msg), "TCPIP_MSG_API")
#if LWIP_MPU_COMPATIBLE
diff --git a/src/include/lwip/sockets.h b/src/include/lwip/sockets.h
index 2522056..0112fbb 100644
--- a/src/include/lwip/sockets.h
+++ b/src/include/lwip/sockets.h
@@ -47,6 +47,7 @@
#include "lwip/err.h"
#include "lwip/inet.h"
#include "lwip/errno.h"
+#include "lwip/sys.h"
#ifdef __cplusplus
extern "C" {
@@ -159,6 +160,32 @@ struct msghdr {
int msg_flags;
};
+#if LWIP_NETCONN_SEM_PER_THREAD
+#define SELECT_SEM_T sys_sem_t*
+#define SELECT_SEM_PTR(sem) (sem)
+#else /* LWIP_NETCONN_SEM_PER_THREAD */
+#define SELECT_SEM_T sys_sem_t
+#define SELECT_SEM_PTR(sem) (&(sem))
+#endif /* LWIP_NETCONN_SEM_PER_THREAD */
+
+/** Description for a task waiting in select */
+struct lwip_select_cb {
+ /** Pointer to the next waiting task */
+ struct lwip_select_cb *next;
+ /** Pointer to the previous waiting task */
+ struct lwip_select_cb *prev;
+ /** readset passed to select */
+ fd_set *readset;
+ /** writeset passed to select */
+ fd_set *writeset;
+ /** unimplemented: exceptset passed to select */
+ fd_set *exceptset;
+ /** don't signal the same semaphore twice: set to 1 when signalled */
+ int sem_signalled;
+ /** semaphore to wake up a task waiting for select */
+ SELECT_SEM_T sem;
+};
+
/* Socket protocol types (TCP/UDP/RAW) */
#define SOCK_STREAM 1
#define SOCK_DGRAM 2
--
2.7.4
_______________________________________________
lwip-users mailing list
lwip-users@nongnu.org
https://lists.nongnu.org/mailman/listinfo/lwip-users
