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

Submitter:  David Lockyer <david_lockyer>
Submitted:  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:  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>
Group administrator
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>
Group 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>
Group administrator
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>
Group administrator
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>
Group 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>
Group administrator
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>
Group 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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

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)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    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.13-02a9.
    Corresponding source code