patchlwIP - A Lightweight TCP/IP stack - Patches: patch #9172, netif: Simplify netif_poll...

 
 

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

patch #9172: netif: Simplify netif_poll implementation

Submitter:  Axel Lin <axellin>
Submitted:  Thu 24 Nov 2016 07:39:02 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
Planned Release:  None

Fri 25 Nov 2016 12:04:46 PM UTC, comment #5: 

As I said, the problem was NOT the while() condition. This does not need to be protected.

The protection + the if() condition were needed to avoid the NULL pointer dereference.

You new patch is OK and more readable than the old code.

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 25 Nov 2016 11:50:13 AM UTC, comment #4: 

Hi Dirk,

I recheck the code again and re-run testing again.
I think the missing lock protect issue as you mentioned is the same for
        while(netif->loop_first != NULL)  {}
and
        do {} while (netif->loop_first != NULL);.

All the reset part of the code accessing netif->loop_first has lock protection and the only missing part is
while (netif->loop_first != NULL).

Attached a revised patch for review.


(file #39067)

Axel Lin <axellin>
Group Member
Fri 25 Nov 2016 11:26:38 AM UTC, comment #3: 

It's not about the while() loop. Problem is:

1) while(netif->loop_first != NULL)
2) Enter loop
3) netif->loop_first becomes NULL from another thread
4) in = in_end = netif->loop_first
5) NULL pointer dereference

Dirk Ziegelmeier <dziegel>
Group administrator
Fri 25 Nov 2016 11:26:11 AM UTC, comment #2: 

Are you sure it was protected before this patch?

The original code likes below:
do {
  SYS_ARCH_PROTECT(lev);
  ...
  SYS_ARCH_UNPROTECT(lev);

} while (netif->loop_first != NULL);

Axel Lin <axellin>
Group Member
Fri 25 Nov 2016 11:18:52 AM UTC, comment #1: 

Invalid, because netif->loop_first is now accessed without locking (SYS_ARCH_PROTECT()...)

Dirk Ziegelmeier <dziegel>
Group administrator
Thu 24 Nov 2016 07:39:02 AM UTC, original submission:  

Use while{} loop instead of do .. while{} loop, then we can avoid NULL test for in pointer in each loop and reduce a level of indent.

Axel Lin <axellin>
Group Member

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Posted a comment)
  • -email is unavailable- added by axellin (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-11-25 dziegel StatusWont Do Done
    2016-11-25 axellin Attached File- Added 0001-netif-Add-proper-lock-protect-for-accessing-netif-lo.patch, #39067
    2016-11-25 dziegel StatusNone Wont Do
        Open/ClosedOpen Closed
    2016-11-24 axellin Attached File- Added 0001-netif-Simplify-netif_poll-implementation.patch, #39064

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code