buglwIP - A Lightweight TCP/IP stack - Bugs: bug #57706, dhcp/acd integration: acd is not...

 
 

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

bug #57706: dhcp/acd integration: acd is not stopped

Submitted by:  Simon Goldschmidt <goldsimon>
Submitted on:  Fri 31 Jan 2020 11:30:20 AM UTC  
 
Category:  DHCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  2.2.0
lwIP version:  git head

( Jump to the original submission)

Fri 14 May 2021 07:29:43 AM UTC, comment #8: 

Yes, thanks for catching that!

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 14 May 2021 05:53:00 AM UTC, comment #7: 
Axel Lin <axellin>
Project Member
Thu 13 May 2021 12:31:46 AM UTC, comment #6: 

Seems missing the code to update *prev.
The *prev is always NULL in adc_remove()?

Axel Lin <axellin>
Project Member
Wed 12 May 2021 07:08:45 PM UTC, comment #5: 

Fixed by adding 'adc_remove()' and calling it when stopping DHCP.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 04 Mar 2021 09:35:05 PM UTC, comment #4: 

Jasper,
sorry for taking more than a year. The whole corona thing changed my life very much regarding time for lwIP. Anyway, I'm trying to catch up right now, and this one is the main blocker for 2.2.0 (and this release is required to dump the 2.1.x branch), so....

I'm still not convinced acd_remove() is not required in dhcp_stop(): the next cll to acd_add() again on the next call to dhcp_start() succeeds because of special check for adding existing acd struct, but

a) we often execute some code just to see that the state is OFF and

b) I think that the counterpart to dhcp_start() is dhcp_stop(). As such, the logical counterpart to calling acd_add() from dhcp_start() should be calling acd_remove() from dhcp_stop().

I admit there's no memory issue when using dhcp_set_struct() (as I do it, too).

As a side note (for the 2.2.0 release): the autoip module does not seem to stop acd, too, so that will have to be fixed before the relase as well :-(

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 06 May 2020 03:15:19 PM UTC, comment #3: 

I agree with the patch made and I applied it on top of the latest master. The attached patch does apply cleanly.

I'm not convinced that we should adc_remove() from dhcp_stop().
The function dhcp_release_and_stop() called by dhcp_stop() does not change anything to the allocated memory.
The dhcp struct is just put in an off state.
So I do not see invalid memory access happening.

However the functions were we do unallocate memory should include something like adc_remove(). I'm thinking of dhcp_cleanup().

I do not use dhcp_cleanup() as I use dhcp_set_struct().
I suggest making an adc_remove function and adding it to dhcp_cleanup.
For the people that use dhcp_set_struct, a warning could be added in comments.

What do you think?

(file #49013)

Jasper Verschueren <jasperv>
Sun 16 Feb 2020 08:16:14 PM UTC, comment #2: 

This is a blocker for 2.2.0, as ACD is one of the major changes between the 2.1.x branch and master.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Sun 16 Feb 2020 08:15:27 PM UTC, comment #1: 

Also, acd_add() has no counterpart acd_remove() -> after calling dhcp_stop(), dhcp->acd is still on the netif->acd_list (which now points to possibly invalid memory)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 31 Jan 2020 11:30:20 AM UTC, original submission:  

acd_stop() is completely missing in dhcp.c.

- if dhcp_stop() is called during ACD probing, the dhcp_conflict_callback() binds the netif to 0.0.0.0

- if dhcp_conflict_callback() is called again later because of ongoing defend, strange things might happen

--> at least acd_stop() must be called from dhcp_release_and_stop() (see attached patch which does not cleanly apply)

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.

 

Attached Files
file #48311:  dhcp_stop_acd.patch added by goldsimon (904B - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by axellin (Posted a comment)
  • -email is unavailable- added by jasperv (Updated the item)
  • -email is unavailable- added by goldsimon (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
    2021-05-12 goldsimon StatusIn Progress => Fixed
        Open/ClosedOpen => Closed
    2021-03-04 goldsimon StatusNone => In Progress
        Assigned toNone => goldsimon
    2020-05-06 jasperv Attached File- => Added 0001-dhcp-stop-acd-module-when-stopping-dhcp.patch, #49013
    2020-02-16 goldsimon Planned ReleaseNone => 2.2.0
    2020-01-31 goldsimon Attached File- => Added dhcp_stop_acd.patch, #48311

    Back to the top


    Powered by Savane 3.6