buglwIP - A Lightweight TCP/IP stack - Bugs: bug #37148, memory pool content is being...

 
 

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

bug #37148: memory pool content is being corrupted after memp_free()

Submitter:  Artem Pisarenko <jblackarty>
Submitted:  Mon 20 Aug 2012 03:19:48 AM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Invalid
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.4.0

Jump to the original submission

Thu 23 Aug 2012 09:44:00 AM UTC, comment #6: 

When I see such a great and flexible pool implementation as lwip has already I become very sad knowing that I have to reinvent the wheel...

Artem Pisarenko <jblackarty>
Thu 23 Aug 2012 09:22:25 AM UTC, comment #5: 


> Ok. It was just suggestion because you asked "Why is pool content overwritten?"


I asked that because I didn't understand what was overwritten. By "memory pool content is being corrupted" I thought you meant the pool's linked list got corrupted.

Changing MEMP_SIZE should work for what you want to do. I won't say it's correct, though - it may or may not work with future changes to memp.c.

Relying on memory to remain untouched while unallocated is not a very portable thing to do anyway, I guess. What you're doing seems like a tradeoff of SRAM size (increased memp pools) for code size (not having to implement the list-keeping of unallocated system items yourself).

I'll close this as invalid now.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 23 Aug 2012 08:13:18 AM UTC, comment #4: 


> That was not an error but an intentional decision to save memory and reuse the data part to keep the linked list pointers

Ok. It was just suggestion because you asked "Why is pool content overwritten?"

But I made modification to lwip sources. I changed

 /* No sanity checks
  * We don't need to preserve the struct memp while not allocated, so we
  * can save a little space and set MEMP_SIZE to 0.
  */
#define MEMP_SIZE           0


to

 /* No sanity checks
  */
#define MEMP_SIZE           LWIP_MEM_ALIGN_SIZE(sizeof(struct memp))


Is it correct ? I'm experiencing memory-related problems right now (particularly, memp_sanity() check). I think this could be their cause.

Artem Pisarenko <jblackarty>
Thu 23 Aug 2012 06:02:44 AM UTC, comment #3: 


> See, 'mem' argument points to memory which holds initialized data.


Yes, it's now clear that this is your actual problem, and I don't think anyone has tried to do that before or there would have been bug reports already.


> But somebody (likely other person)


I think that was me (some years ago).

> forget about memp_free() and defined MEMP_SIZE value as zero.


That was not an error but an intentional decision to save memory and reuse the data part to keep the linked list pointers - this works because the data part is only used when allocated and the linked list pointer is only used when deallocated.

> Moreover, it will corrupt neighbour memory contents on platforms
> where pointer size exceeds minimum size allowed for lwip pool
> element.


I think this can only happen when MEM_ALIGNMENT is not defined correctly. You might be correct though that this wouldn't work on a platform where you don't want/need memory alginment but pointers are big. However, this is a rather theoretical issue, given the fact that lwIP does not have pool items that small...

> What do you recommend to solve this problem gracefully ?


I can't recommend anything right now to solve this gracefully besides adding another config option to support something that lwIP has not yet been designed to do.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 23 Aug 2012 04:50:42 AM UTC, comment #2: 

Ok. If you claim that lwip pools designed to hold only memory and don't care about contents then I have to agree that I use them in wrong way and this bug report is invalid and should be closed.
I'll correct my statement: it makes impossible to use custom defined pools to preallocate initialized system objects.
FreeRTOS allocates object on it's own heap which could be implemented by user. There are three implementations already provided and all of them will cause heap to be fragmented. That's why I have to create objects at start-up and keep their handles (pointers) in pool for reuse. What do you recommend to solve this problem gracefully ?

> Why is pool content overwritten?

See, 'mem' argument points to memory which holds initialized data. Then 'memp' is being assigned to point exactly to the same memory since MEMP_SIZE=0. It's just typecasted and nothing more. Finaly, 'memp->next' assignment rewrites data.
I considered this as a bug because I thought that MEMP_SIZE is provided to shift pointer at least for sizeof(struct memp) before data. But somebody (likely other person) forget about memp_free() and defined MEMP_SIZE value as zero. Moreover, it will corrupt neighbour memory contents on platforms where pointer size exceeds minimum size allowed for lwip pool element.

Artem Pisarenko <jblackarty>
Wed 22 Aug 2012 08:08:58 PM UTC, comment #1: 

Could you go a little more into detail here, please? Why is pool content overwritten? The only thing we do here is to reuse the 'next' pointer: when an allocated item is not in the pool, we don't need the next pointer.

From your last line, my guess is that you are trying to use the memp pools in a wrong way, i.e. initialize objects and put them back into the pool by calling memp_free(xxx, obj) and expecting for obj to remain its state when reallocated by memp_malloc() later. This is invalid: the memp-pools don't hold objects, they only hold memory, the contents of which is not kept between memp_free() and memp_malloc().

> Did anybody tried to implement it before me?


Regarding your statement "preallocate system objects": yes, but in the correct way :-)

Simon Goldschmidt <goldsimon>
Group administrator
Mon 20 Aug 2012 03:19:48 AM UTC, original submission:  

If MEMP_OVERFLOW_CHECK == 0 then

/* No sanity checks
 * We don't need to preserve the struct memp while not allocated, so we
 * can save a little space and set MEMP_SIZE to 0.
 */
#define MEMP_SIZE           0


"Saving a little space" results in situation where following code

void memp_free(memp_t type, void *mem)
{
  ...
  memp = (struct memp *)(void *)((u8_t*)mem - MEMP_SIZE);
  ...
  memp->next = memp_tab[type];
  ...
}

rewrites pool contents !

For example, it makes impossible to use custom defined pools to preallocate system objects in lwip port. (Did anybody tried to implement it before me?)

Artem Pisarenko <jblackarty>

 

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

No files currently attached

 

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 jblackarty (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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-08-23 goldsimon StatusWorks For Me Invalid
        Open/ClosedOpen Closed
    2012-08-22 goldsimon StatusNone Works For Me
        Assigned toNone goldsimon

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code