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

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Fri 31 Jan 2020 11:30:20 AM UTC
   
 
Category:  DHCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  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>
Group administrator
Fri 14 May 2021 05:53:00 AM UTC, comment #7: 
Axel Lin <axellin>
Group 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>
Group 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>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group administrator

 

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

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

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