buglwIP - A Lightweight TCP/IP stack - Bugs: bug #51990, Calling select() from different...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #51990: Calling select() from different threads with MPU enabled triggers memory protection fault

Submitted by:  David Lockyer <david_lockyer>
Submitted on:  Mon 11 Sep 2017 09:58:09 AM UTC  
 
Category:  sockets/netconn Severity:  3 - Normal
Item Group:  Crash Error Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  2.0.2

( Jump to the original submission)

Thu 14 Sep 2017 03:45:13 PM UTC, comment #7: 

Right, thanks for watching ;-)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 13 Sep 2017 09:53:41 PM UTC, comment #6: 

There was a double free on select_cb introduced in the refactoring.  Fixed in 5c0054d8ee90ab89ec830287f00fbcdd27cb475a

Joel Cunningham <jcunningham>
Project Member
Tue 12 Sep 2017 07:39:20 PM UTC, comment #5: 

Pushed with some minor changes. Thanks for the patch, David! Hope it still works for you with my changes ;-)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 12 Sep 2017 06:35:26 PM UTC, comment #4: 

API_SELECT_CB_VAR_ALLOC() lets lwip_select() return ERR_MEM, I'll fix that and push it.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 12 Sep 2017 02:15:43 PM UTC, comment #3: 

mpu_lwip_select_master.patch isn't building on MSVC:

1>e:\projects\lwip\src\api\sockets.c(217): error C2011: 'lwip_select_cb' : 'struct' type redefinition
1>          e:\projects\lwip\src\include\lwip\priv\sockets_priv.h(145) : see declaration of 'lwip_select_cb'

Also, there are some return cases in lwip_select() that aren't calling 'API_SELECT_CB_VAR_FREE(select_cb);' these are new cases from what's on STABLE_2_0_0

Joel Cunningham <jcunningham>
Project Member
Mon 11 Sep 2017 07:29:45 PM UTC, comment #2: 

> Attached git patch set against the STABLE-2_0_0 branch for potential fix


Applying the patch to master fails.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 11 Sep 2017 04:26:12 PM UTC, comment #1: 

David,

I did an initial code review, things looked good.  Only a couple really minors things:

--- 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"

We'll want to keep lwip/sys.h as there is other code in sockets.c using things in the header

@@ -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)
+
+

Extra newline here

 /**
+ * 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
+
+/**

I think for the comment on this option, we'll want to mention 'only needed if you use the sockets API', since select is not available when only using netconn API

Couple notes when porting this fix to master:
1) We'll want to put struct lwip_select_cb and SELECT_ macros in sockets_priv.h rather than public API
2) MEMP_NUM_SELECT_CB memory pool will need to be guarded by #if LWIP_SOCKET_SELECT

Joel Cunningham <jcunningham>
Project Member
Mon 11 Sep 2017 09:58:09 AM UTC, original submission:  

Calling select from different threads with MPU enabled triggers an memory access violation exception in lwip_select(), despite LWIP_MPU_COMPATIBLE being set to 1. This is due to structure select_cb being on the stack, as opposed to in a memory pool.

Attached git patch set against the STABLE-2_0_0 branch for potential fix.

David Lockyer <david_lockyer>

 

Attached Files
file #41788:  mpu_lwip_select_master_2nd_attempt.patch added by david_lockyer (13KiB - text/x-patch - Revised patch for master branch for review)
file #41782:  mpu_lwip_select_STABLE-2_0_0_2nd_attempt.patch added by david_lockyer (12KiB - text/x-patch - Revised 'STABLE-2_0_0' branch patch and 'master' branch patch for review)
file #41783:  mpu_lwip_select_master.patch added by david_lockyer (11KiB - text/x-patch - Revised 'STABLE-2_0_0' branch patch and 'master' branch patch for review)
file #41779:  mpu_lwip_select.patch added by david_lockyer (10KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by jcunningham (Posted a comment)
  • -email is unavailable- added by david_lockyer (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-09-12 goldsimon StatusNone => Fixed
        Assigned toNone => goldsimon
        Open/ClosedOpen => Closed
    2017-09-12 david_lockyer Attached File- => Added mpu_lwip_select_master_2nd_attempt.patch, #41788
    2017-09-12 david_lockyer Attached File- => Added mpu_lwip_select_STABLE-2_0_0_2nd_attempt.patch, #41782
        Attached File- => Added mpu_lwip_select_master.patch, #41783
    2017-09-11 david_lockyer Attached File- => Added mpu_lwip_select.patch, #41779

    Back to the top


    Powered by Savane 3.5