patchlwIP - A Lightweight TCP/IP stack - Patches: patch #10358, dhcp_start : save dhcp memory type...

 
 

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

patch #10358: dhcp_start : save dhcp memory type flag.

Submitter:  MinghaoWang <hao>
Submitted:  Mon 12 Jun 2023 06:07:04 AM UTC
   
 
Category:  DHCP Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
Planned Release:  None

Thu 20 Jul 2023 03:56:17 PM UTC, comment #5: 

Yep, I think you're right. That's what I get for being really tired while coding.

Shawn Silverman <ssilverman>
Thu 20 Jul 2023 11:15:30 AM UTC, comment #4: 

comment #3:

> comment #2:
> >
> > comment #1:
> > > original submission:
> > > > dhcp_start reset all dhcp flags, so a statically allocated struct dhcp which was inited by dhcp_set_struct will try to release its memory in dhcp_cleanup, and that is incorrect.
> > >
> > > dhcp_cleanup() isn't supposed to be called if using dhcp_set_struct(). See the dhcp_cleanup() docs in dhcp.c:
> > >
> > >  * ATTENTION: Only use this when not using dhcp_set_struct() to allocate the
> > >  *            struct dhcp since the memory is passed back to the heap.
> > >
> >
> > I think maybe the doc of dhcp_cleanup() is outdated, because after version 2.2.0 the dhcp_cleanup() was designed to accept struct dhcp which allocated by  dhcp_set_struct(), the dhcp_cleanup() can prevent passing external data to heap by check flag DHCP_FLAG_EXTERNAL_MEM.
> > The problem is this flag will be cleared in dhcp_start(), cause dhcp_cleanup() pass external memory to heap incorrectly, and my patch is used to fix this problem.
> >
>
> I see what you're saying. I'll suggest changing the comment and fixing the flag, however.
>
> Change:


>     /* save dhcp memory type flag, so we can restore it after clear data structure */
>     dhcp_flags = dhcp->flags & DHCP_FLAG_EXTERNAL_MEM;


>
> To this:


>     /* save memory type flag so we can restore it after clearing the structure */
>     dhcp_flags |= DHCP_FLAG_EXTERNAL_MEM;


>


I agree to change the comment, but I disagree on the change of code.
The dhcp_start() was called in many places, and the struct dhcp may be staticly allocated or dynamicly allocated from heap.
We shouldn't set flag DHCP_FLAG_EXTERNAL_MEM directly.

MinghaoWang <hao>
Wed 19 Jul 2023 04:26:11 PM UTC, comment #3: 

comment #2:

>
> comment #1:
> > original submission:
> > > dhcp_start reset all dhcp flags, so a statically allocated struct dhcp which was inited by dhcp_set_struct will try to release its memory in dhcp_cleanup, and that is incorrect.
> >
> > dhcp_cleanup() isn't supposed to be called if using dhcp_set_struct(). See the dhcp_cleanup() docs in dhcp.c:
> >
> >  * ATTENTION: Only use this when not using dhcp_set_struct() to allocate the
> >  *            struct dhcp since the memory is passed back to the heap.
> >
>
> I think maybe the doc of dhcp_cleanup() is outdated, because after version 2.2.0 the dhcp_cleanup() was designed to accept struct dhcp which allocated by  dhcp_set_struct(), the dhcp_cleanup() can prevent passing external data to heap by check flag DHCP_FLAG_EXTERNAL_MEM.
> The problem is this flag will be cleared in dhcp_start(), cause dhcp_cleanup() pass external memory to heap incorrectly, and my patch is used to fix this problem.
>


I see what you're saying. I'll suggest changing the comment and fixing the flag, however.

Change:

    /* save dhcp memory type flag, so we can restore it after clear data structure */
    dhcp_flags = dhcp->flags & DHCP_FLAG_EXTERNAL_MEM;


To this:

    /* save memory type flag so we can restore it after clearing the structure */
    dhcp_flags |= DHCP_FLAG_EXTERNAL_MEM;


Shawn Silverman <ssilverman>
Wed 19 Jul 2023 03:53:09 PM UTC, comment #2: 


comment #1:

> original submission:
> > dhcp_start reset all dhcp flags, so a statically allocated struct dhcp which was inited by dhcp_set_struct will try to release its memory in dhcp_cleanup, and that is incorrect.
>
> dhcp_cleanup() isn't supposed to be called if using dhcp_set_struct(). See the dhcp_cleanup() docs in dhcp.c:
>
>  * ATTENTION: Only use this when not using dhcp_set_struct() to allocate the
>  *            struct dhcp since the memory is passed back to the heap.
>


I think maybe the doc of dhcp_cleanup() is outdated, because after version 2.2.0 the dhcp_cleanup() was designed to accept struct dhcp which allocated by  dhcp_set_struct(), the dhcp_cleanup() can prevent passing external data to heap by check flag DHCP_FLAG_EXTERNAL_MEM.
The problem is this flag will be cleared in dhcp_start(), cause dhcp_cleanup() pass external memory to heap incorrectly, and my patch is used to fix this problem.

MinghaoWang <hao>
Tue 13 Jun 2023 09:34:49 PM UTC, comment #1: 

original submission:

> dhcp_start reset all dhcp flags, so a statically allocated struct dhcp which was inited by dhcp_set_struct will try to release its memory in dhcp_cleanup, and that is incorrect.


dhcp_cleanup() isn't supposed to be called if using dhcp_set_struct(). See the dhcp_cleanup() docs in dhcp.c:

  • ATTENTION: Only use this when not using dhcp_set_struct() to allocate the
  •            struct dhcp since the memory is passed back to the heap.


Shawn Silverman <ssilverman>
Mon 12 Jun 2023 06:07:04 AM UTC, original submission:  

dhcp_start reset all dhcp flags, so a statically allocated struct dhcp which was inited by dhcp_set_struct will try to release its memory in dhcp_cleanup, and that is incorrect.

MinghaoWang <hao>

 

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

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by ssilverman (Posted a comment)
  • -email is unavailable- added by hao (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.

     

    Follows 1 latest change.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-06-12 hao Attached File- Added 0001-dhcp_start-save-dhcp-memory-type.patch, #54830

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code