diff mbox

[U-Boot] malloc: Fix issue with calloc memory possibly being non-zero

Message ID 1289854274-7006-1-git-send-email-galak@kernel.crashing.org
State Accepted
Commit 6163f5b4c8873848ed023054bc401727301ea537
Headers show

Commit Message

Kumar Gala Nov. 15, 2010, 8:51 p.m. UTC
Since we set #define MORECORE_CLEARS 1, the code assumes 'sbrk' always
returns zero'd out memory.  However since its possible that free()
returns memory ack to sbrk() via malloc_trim we could possible get
non-zero'd memory from sbrk() if it allocates back memory that was
previously freed to it.

There are two possible solutions to this problem.
1. change #define MORECORE_CLEARS 0
2. memset to zero memory returned to sbrk.

We go with the second since the sbrk being called to free up memory
should be pretty rare.

The following code problems an example test to show the issue.  This
test code was inserted right after the call to mem_malloc_init(). we
could possible get non-zero'd memory from sbrk() if it allocates back
memory that was previously freed to it.

There are two possible solutions to this problem.
1. change #define MORECORE_CLEARS 0
2. memset to zero memory returned to sbrk.

We go with the second since the sbrk being called to free up memory
should be pretty rare.

The following code problems an example test to show the issue.  This
test code was inserted right after the call to mem_malloc_init().

...
       u8 *p2;
       int i;

       printf("MALLOC TEST\n");
       p1 = malloc(135176);
       printf("P1 = %p\n", p1);
       memset(p1, 0xab, 135176);

       free(p1);
       p2 = calloc(4097, 1);
       printf("P2 = %p %p\n", p2, p2 + 4097);

       for (i = 0; i < 4097; i++) {
	       if (p2[i] != 0)
		       printf("miscompare at byte %d got %x\n", i, p2[i]);

       free(p2);
       printf("END MALLOC TEST\n\n");
...

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 common/dlmalloc.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

Comments

Mike Frysinger Nov. 15, 2010, 9:52 p.m. UTC | #1
On Monday, November 15, 2010 15:51:14 Kumar Gala wrote:
> returns memory ack to sbrk() via malloc_trim we could possible get

"ack" -> "back"

> The following code problems an example test to show the issue.

i dont understand this sentence

> we
> could possible get non-zero'd memory from sbrk() if it allocates back
> memory that was previously freed to it.
> 
> There are two possible solutions to this problem.
> 1. change #define MORECORE_CLEARS 0
> 2. memset to zero memory returned to sbrk.
> 
> We go with the second since the sbrk being called to free up memory
> should be pretty rare.
> 
> The following code problems an example test to show the issue.  This
> test code was inserted right after the call to mem_malloc_init().

this looks like the changelog is partially duplicated
-mike
Wolfgang Denk Nov. 15, 2010, 10:06 p.m. UTC | #2
Dear Kumar Gala,

In message <1289854274-7006-1-git-send-email-galak@kernel.crashing.org> you wrote:
> Since we set #define MORECORE_CLEARS 1, the code assumes 'sbrk' always
> returns zero'd out memory.  However since its possible that free()
> returns memory ack to sbrk() via malloc_trim we could possible get
> non-zero'd memory from sbrk() if it allocates back memory that was
> previously freed to it.

I confirm that the test case works fine here, too.  So:

Tested-by: Wolfgang Denk <wd@denx.de>


But the commit message needs serious rework before this can get
applied.

Best regards,

Wolfgang Denk
Kumar Gala Nov. 16, 2010, 12:35 a.m. UTC | #3
On Nov 15, 2010, at 4:06 PM, Wolfgang Denk wrote:

> Dear Kumar Gala,
> 
> In message <1289854274-7006-1-git-send-email-galak@kernel.crashing.org> you wrote:
>> Since we set #define MORECORE_CLEARS 1, the code assumes 'sbrk' always
>> returns zero'd out memory.  However since its possible that free()
>> returns memory ack to sbrk() via malloc_trim we could possible get
>> non-zero'd memory from sbrk() if it allocates back memory that was
>> previously freed to it.
> 
> I confirm that the test case works fine here, too.  So:
> 
> Tested-by: Wolfgang Denk <wd@denx.de>
> 
> 
> But the commit message needs serious rework before this can get
> applied.

Oops, will fix and repost.

- k
diff mbox

Patch

diff --git a/common/dlmalloc.c b/common/dlmalloc.c
index 4871f4b..e9bab09 100644
--- a/common/dlmalloc.c
+++ b/common/dlmalloc.c
@@ -1511,6 +1511,13 @@  void *sbrk(ptrdiff_t increment)
 	ulong old = mem_malloc_brk;
 	ulong new = old + increment;
 
+	/*
+	 * if we are giving memory back make sure we clear it out since
+	 * we set MORECORE_CLEARS to 1
+	 */
+	if (increment < 0)
+		memset((void *)new, 0, -increment);
+
 	if ((new < mem_malloc_start) || (new > mem_malloc_end))
 		return (void *)MORECORE_FAILURE;