mem_malloc bug

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

mem_malloc bug

Tom Hennen
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 */
}

I have an idea why this is failing (and a possible method to fix it),
but I wanted to make sure this wasn't already known and expected.

Thoughts?

-Tom


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: mem_malloc bug

Kieran Mansley
On Tue, 2006-10-03 at 16:16 -0400, Tom Hennen wrote:

> I have an idea why this is failing (and a possible method to fix it),
> but I wanted to make sure this wasn't already known and expected.

Doesn't sound familiar to me, but it's a long time since I looked at
that code, and so it's probably all changed in the mean time!  Please go
ahead and elaborate with a fix if possible.

Kieran



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: mem_malloc bug

Christiaan Simons
In reply to this post by Tom Hennen

Hi Tom,

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

Yikes. I'm currently working on some SNMP agent
stuff that heavily uses mem_malloc()...

> I have an idea why this is failing (and a possible method to fix it),
> but I wanted to make sure this wasn't already known and expected.

I wasn't aware of this. mem_malloc() did seem to work ok for me,
but I may have missed this odd corner.

Can you please share your thoughts with us?

Christiaan Simons

Hardware Designer
Axon Digital Design

http://www.axon.tv



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: mem_malloc bug

Tom Hennen
Well, from what I can tell this bug has existed for the entire history
of mem.c in CVS.

The bug seems to be a result of line 256, where the size of the 'mem'
struct is counted twice.  Instead of:

mem->next - (ptr + SIZEOF_STRUCT_MEM) >= size + SIZEOF_STRUCT_MEM)

It should read:

mem->next - ptr >= size + SIZEOF_STRUCT_MEM)

That isn't the only fix required however.  The code that updates mem2
assumes that mem2 is in some region of memory that isn't currently
allocated.  However, with the fix above (and the test case I provided
earlier) mem2 will point to a valid, already existing 'mem' struct and
so it shouldn't be updated.

To solve this problem after figuring out the value of ptr2 there
should be a check to see if mem->next points to ptr2, if it does then
mem2 is a valid struct and should be updated.  If it doesn't point to
ptr2 then it should be safe to create a new 'mem' struct at that
location.  Note that this solution ignores the problem of what would
happen if ptr2 actually pointed half-way into the existing 'mem'
struct.

On 10/4/06, Christiaan Simons <[hidden email]> wrote:

>
> Hi Tom,
>
> > I've run across what I believe to be a bug in mem_malloc.
>
> Yikes. I'm currently working on some SNMP agent
> stuff that heavily uses mem_malloc()...
>
> > I have an idea why this is failing (and a possible method to fix it),
> > but I wanted to make sure this wasn't already known and expected.
>
> I wasn't aware of this. mem_malloc() did seem to work ok for me,
> but I may have missed this odd corner.
>
> Can you please share your thoughts with us?
>
> Christiaan Simons
>
> Hardware Designer
> Axon Digital Design
>
> http://www.axon.tv
>
>
>
> _______________________________________________
> lwip-users mailing list
> [hidden email]
> http://lists.nongnu.org/mailman/listinfo/lwip-users
>


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: mem_malloc bug

Kieran Mansley
On Wed, 2006-10-04 at 09:57 -0400, Tom Hennen wrote:
> Well, from what I can tell this bug has existed for the entire history
> of mem.c in CVS.
>
> The bug seems to be a result of line 256, where the size of the 'mem'
> struct is counted twice.  Instead of:
>
> mem->next - (ptr + SIZEOF_STRUCT_MEM) >= size + SIZEOF_STRUCT_MEM)

That does rather look like a bug, and one that should be fixed, but I
can see how it survived for so long - the only side effect is that you
can't use the last little bit of your memory, and so few people are
likely to hit this, or notice if they do as it doesn't affect stability.

> It should read:
>
> mem->next - ptr >= size + SIZEOF_STRUCT_MEM)
>
> That isn't the only fix required however.  The code that updates mem2
> assumes that mem2 is in some region of memory that isn't currently
> allocated.  However, with the fix above (and the test case I provided
> earlier) mem2 will point to a valid, already existing 'mem' struct and
> so it shouldn't be updated.

Does it matter if a new one is made rather than updating the one in
place?  It's not clear from the above what happens as a result, or if
it's just a bit inefficient the way it's currently written.

Would you like to provide a patch for these changes?

Thanks

Kieran



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: mem_malloc bug

Tom Hennen
"that you can't use the last little bit of your memory..."

That's how I ran across it in fact.  I wasn't able to use the entire
TCP_SNDBUF as I was running out of memory before TCP_SNDBUF was full.


"Does it matter if a new one is made rather than updating the one in place?"

The existing one should be updated.  Otherwise 'mem2->next =
mem->next' will cause mem2 to point to itself and 'mem2->used = 0'
will cause this block (which *is* used) to appear unused, causing an
error when it is freed (if not sooner).

I'm not sure if I'll be able to submit a patch (i.e. if my employer
will let me).


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: mem_malloc bug

Kieran Mansley
On Wed, 2006-10-04 at 18:25 -0400, Tom Hennen wrote:

> I'm not sure if I'll be able to submit a patch (i.e. if my employer
> will let me).

OK, in that case, could you submit a bug on savannah, and point it at
this discussion on the mailing list so this issue doesn't get forgotten,
and someone will hopefully get round to fixing it in the near future.  

Thanks for taking the time to get to the bottom of this and letting us
know.

Kieran



_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users
Reply | Threaded
Open this post in threaded view
|

Re: mem_malloc bug

Tom Hennen
Done.  Hopefully I'll get some time to work on a patch...

On 10/5/06, Kieran Mansley <[hidden email]> wrote:

> On Wed, 2006-10-04 at 18:25 -0400, Tom Hennen wrote:
>
> > I'm not sure if I'll be able to submit a patch (i.e. if my employer
> > will let me).
>
> OK, in that case, could you submit a bug on savannah, and point it at
> this discussion on the mailing list so this issue doesn't get forgotten,
> and someone will hopefully get round to fixing it in the near future.
>
> Thanks for taking the time to get to the bottom of this and letting us
> know.
>
> Kieran
>
>
>
> _______________________________________________
> lwip-users mailing list
> [hidden email]
> http://lists.nongnu.org/mailman/listinfo/lwip-users
>


_______________________________________________
lwip-users mailing list
[hidden email]
http://lists.nongnu.org/mailman/listinfo/lwip-users