buglwIP - A Lightweight TCP/IP stack - Bugs: bug #17922, mem_malloc bug

 
 

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

bug #17922: mem_malloc bug

Submitter:  None
Submitted:  Thu 05 Oct 2006 09:29:25 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  None
Open/Closed:  Closed Planned Release:  None
lwIP version:  None

Jump to the original submission

Sun 13 May 2007 02:08:01 PM UTC, comment #25: 

I'm now closing this as fixed, someone reopen it if they find it to be unstable/unsatisfying.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 08 May 2007 10:51:01 AM UTC, comment #24: 


>Maybe an assert would be better then, just to cover that base, as clearly in the current code it should never happen.


You're right. I've checked that in.

Simon Goldschmidt <goldsimon>
Group administrator
Tue 08 May 2007 09:08:00 AM UTC, comment #23: 

re the patch: I have no reason to doubt it works. Silence is silent agreement :-).

Re mem_alloc checking for growing memory... While it seems plausible (ditto a check for illegal memory further up), the only user of mem_realloc is pbuf_realloc, and it doesn't check the return value at all. But then pbuf_realloc does its own check that the space doesn't grow.

Maybe an assert would be better then, just to cover that base, as clearly in the current code it should never happen.

Jonathan Larmour <jifl>
Group Member
Mon 07 May 2007 07:52:21 PM UTC, comment #22: 

Ok, checked it in. I hope I don't hear anything about it which would mean it works ;-)

While I'm at it: shouldn't mem_realloc() return NULL if the "if (newsize + SIZEOF_STRUCT_MEM + MIN_SIZE < size)" check (that checks that memory only gets smaller, not bigger) fails???

Simon Goldschmidt <goldsimon>
Group administrator
Mon 07 May 2007 07:14:40 PM UTC, comment #21: 

Since noone is answering, I'm checking the patch in, now and setting it as default. We do have a bug here and I want that solved. As it works for me, that way, at least I get some feedback if it's wrong ;-)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 03 May 2007 05:53:38 PM UTC, comment #20: 

Can anyone comment whether the patch in file #12598 works correct?

It still can be advanced so that on near fit, mem->next will get the rest remaining between mem and the former mem->next, resulting in a little better ram usage.

I'll attach a new patch file for this. I'd like to have some comments, please, so we can move on with this! If I don't get any for some days, I'll check it in after some testing as I think it's better than now, at least!

(file #12681)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 26 Apr 2007 06:53:52 PM UTC, comment #19: 

Christiaan, I've tried the 3 different versions with the TCP echo server included in contrib/ports/unix/proj/unixsim/apps/tcpecho.c under windows. It works for hours using Adam's original mem_malloc() and my modified version.

For TCP echo test, your version works OK, too.
However, an assert triggers in mem_free() (LWIP_ASSERT("mem_free: mem->used", mem->used);) with ARP queueing, and it seems like the ram region is used twice and set to unused again as the pbuf information of the pbuf queued in ARP is correct.

Did you test my patch? (file #12598)

Simon Goldschmidt <goldsimon>
Group administrator
Thu 26 Apr 2007 07:02:58 AM UTC, comment #18: 


> What exactly did you see to say it broke TCP?


I did some TCP echo using small (1k) files in an infinite loop.
(while (( 1 )); do ./echop; done), and the echo fails after a few cycles using my malloc. When using the original mem_malloc() it all works, more or less. TCP did give me some headaches, but I couldn't explain.

I also vaguely remember the DHCP failed on my target using my changes to mem_malloc(), so therefore I kept the code disabled.

> TCP should not depend on mem_malloc() implementation (other than getting memory :)


True! I find it quite odd, but I couldn't trace the root cause.
Maybe I made some other mistake. We reviewed my stuff here, but we couldn't explain why my code fails. Maybe a combination of bugs.

Christiaan Simons <christiaans>
Group Member
Wed 25 Apr 2007 08:04:07 PM UTC, comment #17: 

OK, this time I modified Christiaans version of mem_malloc(). This works for me:
- I can malloc the whole (MEM_SIZE - SIZEOF_STRUCT_MEM)
- "Toms test" (see below) passes
- No exceptions when running the stack with UDP / TCP apps (except memory allocation failures -> whole mem is used sometimes!)

I'll attach the patch and look forward to your comments!

(file #12598)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 25 Apr 2007 03:22:32 PM UTC, comment #16: 


>However i didn't enable it since it seem to break TCP, but maybe that is because it assumes some behavior of the broken mem_malloc(). I did not have the time to trace that issue.


What exactly did you see to say it broke TCP? TCP should not depend on mem_malloc() implementation (other than getting memory :)

I noticed though that your patch did some funny things because first you set mem->next to mem2 and then mem2->next to mem->next. I'll investigate if that is the problem...

>No idea. Didn't look at the stats, and yes it seems wrong too.

I saw it was already that way in the first version checked into svannah CVS... Maybe noone ever really cared, but I think I'll change that, too.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 25 Apr 2007 11:44:28 AM UTC, comment #15: 

No idea. Didn't look at the stats, and yes it seems wrong too.

Also note some variable names are a bit misleading here: ptr and ptr2 are actually integer offets of type mem_size_t.

Christiaan Simons <christiaans>
Group Member
Wed 25 Apr 2007 10:51:57 AM UTC, comment #14: 

As I said, Jonathan seems to have more trust in me than I do ;-)

Does anybody know why it includes

if (lwip_stats.mem.max < ptr2) {
  lwip_stats.mem.max = ptr2;
}

instead of

if (lwip_stats.mem.max < lwip_stats.mem.used) {
  lwip_stats.mem.max = lwip_stats.mem.used;
}

??? That was included in lwIP 1.1.0 already, before any changes from this bug report were made.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 25 Apr 2007 07:30:37 AM UTC, comment #13: 

Please have a look at what i've tried before (change the #if 1 into 0). There are two cases in malloc.

1. you need to split the first free (too large) block into two pcs
2. the first block is only large enough to fit one block, so you may not allocate a new empty header+block as a trailer.

I think the original code did not handle these cases properly,
my changed code passes the testcase provided by Tom.

However i didn't enable it since it seem to break TCP, but maybe that is because it assumes some behavior of the broken mem_malloc(). I did not have the time to trace that issue.

So maybe just enable my fix, and continue from there.

Christiaan Simons <christiaans>
Group Member
Tue 24 Apr 2007 07:16:07 PM UTC, comment #12: 


>Since you've tested it, I'm sure that's good enough to go with.


Seems you have more trust in me than I have myself ;-)

I'll be running some more tests this week. Also, Tom (the original poster) has some more thoughts in http://lists.nongnu.org/archive/html/lwip-users/2006-10/msg00001.html So this may still need some thinking...

Simon Goldschmidt <goldsimon>
Group administrator
Tue 24 Apr 2007 03:40:48 PM UTC, comment #11: 

From looking at it, it seems plausible. Since you've tested it, I'm sure that's good enough to go with.

Jonathan Larmour <jifl>
Group Member
Tue 24 Apr 2007 03:21:34 PM UTC, comment #10: 


>You shouldn't try making life so hard for yourself.
>Try  to fix the broken mem_malloc() first.


I've had a quick look into mem.c (which I don't use myself, only to make lwIP more bug-free :)

I'm attaching a patch that (for me) solves this case. PLEASE: comment so that we can finally close this!

(file #12588)

Simon Goldschmidt <goldsimon>
Group administrator
Wed 11 Apr 2007 03:11:55 PM UTC, comment #9: 

Maybe it might be worth tweaking mem.h to be something like:
#if MEM_LIBC_MALLOC
/* aliases for C library malloc() */
#ifndef mem_init
# define mem_init()
#endif
#ifndef mem_free
# define mem_free(x) free(x)
#endif
#ifndef mem_malloc
# define mem_malloc(x) malloc(x)
#endif
#ifndef mem_realloc
# define mem_realloc(x, size) realloc(x,size)
#endif

This way a port can choose to provide its own function, even if it's only a wrapper around malloc, adding some locking.

What do you think?

To answer the other part of the initial report: yes there are systems with string functions but not a heap implementation.

Jonathan Larmour <jifl>
Group Member
Wed 11 Apr 2007 11:34:13 AM UTC, comment #8: 


>some bare-bone raw-API micro's simply don't have semaphores.


Yes, but for raw-API-only you normally don't need to protect mem_malloc() since you don't have multiple threads (and if you have multiple threads, normally you have semaphores). The reason for this is that in interrupt context (which in a thread-less design is the only source of parallelism), only PBUF_POOL types should be allocated, so our heap is not touched.

Anyway, this is no problem for me, since I'm not using the heap. Just wanted to point out a possible problem...

Simon Goldschmidt <goldsimon>
Group administrator
Wed 11 Apr 2007 10:43:44 AM UTC, comment #7: 

Certain implementations have (empty) __malloc_lock(), __malloc_unlock() function external to the actual malloc for this purpose.

Looks cleaner to me than assuming a system actually has semaphores, some bare-bone raw-API micro's simply don't have semaphores.

Not solving this case. Bye.

Christiaan Simons <christiaans>
Group Member
Wed 11 Apr 2007 09:51:06 AM UTC, comment #6: 

When using MEM_LIBC_MALLOC, at least on some platforms, there will be a multithreading problem since malloc() is by default not protected. Is this a problem? Should we have a semaphore around the calls to malloc/free/realloc as we have it in our normal mem_malloc() implementation?

Apart from that, why do we have our own mem_malloc() if we by default include <string.h> in many files? Are there toolchains including the 'string.h'-functions which lack the 'stdlib.h'-functions?

Simon Goldschmidt <goldsimon>
Group administrator
Tue 23 Jan 2007 12:45:33 PM UTC, comment #5: 

In my opinion the following Code does the same as using “_attribute_ ((aligned (MEM_ALIGNMENT)))”. I’m not an expert in C programming and lwip so there might be another problem.


#if MEM_ALIGNMENT == 1
static u8_t ram[MEM_SIZE + sizeof(struct mem) + MEM_ALIGNMENT];
#elif MEM_ALIGNMENT == 2
static u16_t ram16[(MEM_SIZE + sizeof(struct mem) + MEM_ALIGNMENT)>>1];
static u8_t ram = (u8_t )ram16;
#elif MEM_ALIGNMENT == 4
static u32_t ram32[(MEM_SIZE + sizeof(struct mem) + MEM_ALIGNMENT)>>2];
static u8_t ram = (u8_t )ram32;
#elif MEM_ALIGNMENT == 8
static u64_t ram64[(MEM_SIZE + sizeof(struct mem) + MEM_ALIGNMENT)>>3];
static u8_t ram = (u8_t )ram64;
#else
#error "unhandled MEM_ALIGNMENT size"
#endif

Andreas Funcke <funcke>
Mon 22 Jan 2007 08:17:56 AM UTC, comment #4: 

I can't recommend your fix since it highly depends on gcc specific attributes. AFAIK attributes aren't ANSI C. My fix doesn't guarantee alignment, since it depends on the alignment of the struct ram_heap, usually the alignment of the first member of this struct.

Actually we need to use a typedef from cc.h to tell us which is the aligment type for the machine we're running on, and use this type for alignment of the ram_heap[].

My other changes didn't seem to work for me on the c16x so, some more work needs to be done here. Any volunteers here?

Christiaan Simons <christiaans>
Group Member
Sun 21 Jan 2007 09:05:49 AM UTC, comment #3: 

Hello, I’m a newbie and had problems with the alignment in mem.c too. I realized a fix which is working in the following way. I hope this information will help others too.

mem.c (line 68)
static struct mem *ram_end;
#if 0
/* Adam original */
static u8_t ram[MEM_SIZE + sizeof(struct mem) + MEM_ALIGNMENT];
#else
// Andreas alignment fix
static _attribute_ ((aligned (MEM_ALIGNMENT))) u8_t ram[MEM_SIZE + sizeof(struct mem) + MEM_ALIGNMENT];
/*
// Christiaan alignment fix
static u8_t *ram;
static struct mem ram_heap[1 + ( (MEM_SIZE + sizeof(struct mem) - 1) / sizeof(struct mem))];
*/
#endif

Andreas Funcke <funcke>
Fri 01 Dec 2006 03:50:53 PM UTC, comment #2: 

Fix in CVS doesn't work for me, but I can't find the reason why it fails.

Therefore I introduced a workaround option MEM_LIBC_MALLOC. Set to 1 if you choose to use you local libc malloc() instead.

I'll keep this bug open for your input (at lowered severity).

Christiaan Simons <christiaans>
Group Member
Mon 27 Nov 2006 02:31:54 PM UTC, comment #1: 

Fix in CVS. Please confirm solution, this issue is blocking current release plan.

Christiaan Simons <christiaans>
Group Member
Thu 05 Oct 2006 09:29:25 PM UTC, original submission:  

I've run across what I believe to be a bug in mem_malloc.

Basically, if you allocate all the memory in multiple blocks, then free the first block you allocated, you cannot allocate a new block of the same size.

The following code fails:

test() {
 void * ptr1
 void * cur;

 /** allocate the first block */
 ptr1 = mem_malloc(200);
 ASSERT(ptr1); /* <--- this succeeds */

 /** allocate all the remaining blocks */
 do {
   cur = mem_malloc(200);
 } while (cur);

 /** free the first block */
 mem_free(ptr1);

 /** now attempt to re-allocate the 200 bytes just freed */
 ptr1 = mem_malloc(200);
 ASSERT(ptr1);  /* <---- this FAILS */
}

Please refer to this thread for more information:
http://lists.nongnu.org/archive/html/lwip-users/2006-10/msg00001.html

Anonymous

 

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

Attached Files
file #12681:  mem.c3.patch added by goldsimon (5KiB - text/x-patch)
file #12598:  mem.c2.patch added by goldsimon (4KiB - text/x-patch)
file #12588:  mem.c.patch added by goldsimon (663B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by funcke (Posted a comment)
  • -email is unavailable- added by christiaans (Posted a comment)
  •  

    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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-05-13 goldsimon StatusNone Fixed
        Open/ClosedOpen Closed
    2007-05-03 goldsimon Attached File- Added mem.c3.patch, #12681
    2007-04-25 goldsimon Attached File- Added mem.c2.patch, #12598
    2007-04-24 goldsimon Attached File- Added mem.c.patch, #12588
    2007-04-11 christiaans Assigned tochristiaans None
    2006-12-01 christiaans Severity4 - Important 3 - Normal
    2006-11-20 christiaans Severity3 - Normal 4 - Important
        Assigned toNone christiaans

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code