Commit Message
glibc testing would sometimes generate this failure for me:
FAIL: malloc/tst-malloc-usable.out
malloc_usable_size: expected 7 but got 11
I found that the checking code fills the unused portion of a memory
chunk (end to front) with a chain of length-prefixed blocks
followed by a "magic" byte (a hash of the chunk's address).
Verification of the unused portion reads each block advancing via
the length bytes. Sometimes a length byte would be interpreted as
the magic hash. This caused the scan to terminate early and report a
larger usable size than actual.
My fix is to terminate the chain with a zero-length block.
The chain is still followed by the magic hash but there is now no ambiguity.
Tested with a mips-linux build where I noticed the problem.
Also tested with a x86_64 native make-check. Results were unchanged.
OK to commit?
Comments
James Lemke <jwlemke@codesourcery.com> writes:
> +/* Visualize the chunk as being partitioned into blocks of 255 bytes from the
> + highest address of the chunk, downwards. The end of each block tells us
> + the size of that block, up to the actual size of the requested memory.
> + The last block has a length of zero and is followed by the magic byte.
> + Our magic byte is right at the end of the requested size. If we don't
> + reach it with this iteration we have witnessed a memory corruption. */
> static size_t
> malloc_check_get_size (mchunkptr p)
> {
> - size_t size;
> + size_t total_sz, size;
> unsigned char c;
> unsigned char magic = MAGICBYTE (p);
>
> assert (using_malloc_checking == 1);
>
> - for (size = chunksize (p) - 1 + (chunk_is_mmapped (p) ? 0 : SIZE_SZ);
> - (c = ((unsigned char *) p)[size]) != magic;
> + /* Validate the length-byte chain. */
> + total_sz = chunksize (p) + (chunk_is_mmapped (p) ? 0 : SIZE_SZ);
> + for (size = total_sz - 1;
> + (c = ((unsigned char *) p)[size]) != 0;
> size -= c)
> {
> - if (c <= 0 || size < (c + 2 * SIZE_SZ))
> - {
> - malloc_printerr (check_action, "malloc_check_get_size: memory corruption",
> - chunk2mem (p));
> - return 0;
> - }
> + if (size - c <= 2 * SIZE_SZ)
If c > size then the difference wraps around.
Andreas.
As this bug is user-visible in past releases, it should be filed in
Bugzilla if not already there (and the [BZ #N] notation used in the
ChangeLog entry, and the bug added to the list of fixed bugs in NEWS when
checking in the fix).
On 11/11/2014 03:32 PM, Joseph Myers wrote:
> As this bug is user-visible in past releases, it should be filed in
> Bugzilla if not already there (and the [BZ #N] notation used in the
> ChangeLog entry, and the bug added to the list of fixed bugs in NEWS when
> checking in the fix).
I have opened BZ 17581.
I'll mention it in ChangeLog & NEWS as you suggested.
On 11/11/2014 03:19 PM, Andreas Schwab wrote:
>> - if (c <= 0 || size < (c + 2 * SIZE_SZ))
>> >- {
>> >- malloc_printerr (check_action, "malloc_check_get_size: memory corruption",
>> >- chunk2mem (p));
>> >- return 0;
>> >- }
>> >+ if (size - c <= 2 * SIZE_SZ)
>> >+ break;
>
> If c > size then the difference wraps around.
That would indicate memory corruption and the loop would terminate,
which it should.
However, if you think it's clearer, I can re-write the 3 occurrences of
this test as:
if (size <= c + 2 * SIZE_SZ)
Otherwise OK?
James Lemke <jwlemke@codesourcery.com> writes:
> On 11/11/2014 03:19 PM, Andreas Schwab wrote:
>>> - if (c <= 0 || size < (c + 2 * SIZE_SZ))
>>> >- {
>>> >- malloc_printerr (check_action, "malloc_check_get_size: memory corruption",
>>> >- chunk2mem (p));
>>> >- return 0;
>>> >- }
>>> >+ if (size - c <= 2 * SIZE_SZ)
>>> >+ break;
>>
>> If c > size then the difference wraps around.
>
> That would indicate memory corruption and the loop would terminate,
> which it should.
This condition will not terminate it, and the next iteration will cause
size to wrap around.
Andreas.
On 11/11/2014 04:31 PM, Andreas Schwab wrote:
>>>> - if (c <= 0 || size < (c + 2 * SIZE_SZ))
>>>>> >>> >- {
>>>>> >>> >- malloc_printerr (check_action, "malloc_check_get_size: memory corruption",
>>>>> >>> >- chunk2mem (p));
>>>>> >>> >- return 0;
>>>>> >>> >- }
>>>>> >>> >+ if (size - c <= 2 * SIZE_SZ)
>>>>> >>> >+ break;
>>> >>
>>> >>If c > size then the difference wraps around.
>> >
>> >That would indicate memory corruption and the loop would terminate,
>> >which it should.
> This condition will not terminate it, and the next iteration will cause
> size to wrap around.
Err, yes. size is unsigned so you are correct. Thanks for the input.
I will re-write the 3 instances of this test as:
if (size <= c + 2 * SIZE_SZ)
Otherwise OK?
Ping!
Any opinions about committing this?
Cheers, Jim.
https://sourceware.org/ml/libc-alpha/2014-11/msg00219.html
https://sourceware.org/ml/libc-alpha/2014-11/msg00470.html
On 25 November 2014 at 21:52, James Lemke <jwlemke@mentor.com> wrote:
> Ping!
> Any opinions about committing this?
> Cheers, Jim.
>
> https://sourceware.org/ml/libc-alpha/2014-11/msg00219.html
> https://sourceware.org/ml/libc-alpha/2014-11/msg00470.html
This looks ok to me, although there are some minor style issues, e.g.:
There should be a space after casts.
There's a bracket on its own that should be moved onto the line above.
A couple of if statements have some leading space that should not be there.
2014-11-11 James Lemke <jwlemke@codesourcery.com>
* malloc/hooks.c
(mem2mem_check): Add a terminator to the chain of checking blocks.
(malloc_check_get_size): Use it here.
(mem2chunk_check): Ditto.
@@ -90,31 +90,35 @@ __malloc_check_init (void)
#define MAGICBYTE(p) ((((size_t) p >> 3) ^ ((size_t) p >> 11)) & 0xFF)
-/* Visualize the chunk as being partitioned into blocks of 256 bytes from the
- highest address of the chunk, downwards. The beginning of each block tells
- us the size of the previous block, up to the actual size of the requested
- memory. Our magic byte is right at the end of the requested size, so we
- must reach it with this iteration, otherwise we have witnessed a memory
- corruption. */
+/* Visualize the chunk as being partitioned into blocks of 255 bytes from the
+ highest address of the chunk, downwards. The end of each block tells us
+ the size of that block, up to the actual size of the requested memory.
+ The last block has a length of zero and is followed by the magic byte.
+ Our magic byte is right at the end of the requested size. If we don't
+ reach it with this iteration we have witnessed a memory corruption. */
static size_t
malloc_check_get_size (mchunkptr p)
{
- size_t size;
+ size_t total_sz, size;
unsigned char c;
unsigned char magic = MAGICBYTE (p);
assert (using_malloc_checking == 1);
- for (size = chunksize (p) - 1 + (chunk_is_mmapped (p) ? 0 : SIZE_SZ);
- (c = ((unsigned char *) p)[size]) != magic;
+ /* Validate the length-byte chain. */
+ total_sz = chunksize (p) + (chunk_is_mmapped (p) ? 0 : SIZE_SZ);
+ for (size = total_sz - 1;
+ (c = ((unsigned char *) p)[size]) != 0;
size -= c)
{
- if (c <= 0 || size < (c + 2 * SIZE_SZ))
- {
- malloc_printerr (check_action, "malloc_check_get_size: memory corruption",
- chunk2mem (p));
- return 0;
- }
+ if (size - c <= 2 * SIZE_SZ)
+ break;
+ }
+ if ( c != 0 || ((unsigned char *)p)[--size] != magic)
+ {
+ malloc_printerr (check_action, "malloc_check_get_size: memory corruption",
+ chunk2mem (p));
+ return 0;
}
/* chunk2mem size. */
@@ -130,22 +134,24 @@ mem2mem_check (void *ptr, size_t sz)
{
mchunkptr p;
unsigned char *m_ptr = ptr;
- size_t i;
+ size_t user_sz, block_sz, i;
if (!ptr)
return ptr;
p = mem2chunk (ptr);
- for (i = chunksize (p) - (chunk_is_mmapped (p) ? 2 * SIZE_SZ + 1 : SIZE_SZ + 1);
- i > sz;
- i -= 0xFF)
+ user_sz = chunksize (p) + (chunk_is_mmapped (p) ? 0 : SIZE_SZ);
+ user_sz -= 2 * SIZE_SZ;
+ for (i = user_sz - 1; i > sz; i -= block_sz)
{
- if (i - sz < 0x100)
- {
- m_ptr[i] = (unsigned char) (i - sz);
- break;
- }
- m_ptr[i] = 0xFF;
+ block_sz = i - (sz + 1);
+ if (block_sz > 0xff)
+ block_sz = 0xff;
+
+ m_ptr[i] = (unsigned char) block_sz;
+
+ if (block_sz == 0)
+ break;
}
m_ptr[sz] = MAGICBYTE (p);
return (void *) m_ptr;
@@ -166,11 +172,12 @@ mem2chunk_check (void *mem, unsigned char **magic_p)
return NULL;
p = mem2chunk (mem);
+ sz = chunksize (p);
+ magic = MAGICBYTE (p);
if (!chunk_is_mmapped (p))
{
/* Must be a chunk in conventional heap memory. */
int contig = contiguous (&main_arena);
- sz = chunksize (p);
if ((contig &&
((char *) p < mp_.sbrk_base ||
((char *) p + sz) >= (mp_.sbrk_base + main_arena.system_mem))) ||
@@ -180,12 +187,13 @@ mem2chunk_check (void *mem, unsigned char **magic_p)
next_chunk (prev_chunk (p)) != p)))
return NULL;
- magic = MAGICBYTE (p);
- for (sz += SIZE_SZ - 1; (c = ((unsigned char *) p)[sz]) != magic; sz -= c)
+ for (sz += SIZE_SZ - 1; (c = ((unsigned char *) p)[sz]) != 0; sz -= c)
{
- if (c <= 0 || sz < (c + 2 * SIZE_SZ))
- return NULL;
+ if (sz - c <= 2 * SIZE_SZ)
+ break;
}
+ if ( c != 0 || ((unsigned char *)p)[--sz] != magic)
+ return NULL;
}
else
{
@@ -201,15 +209,17 @@ mem2chunk_check (void *mem, unsigned char **magic_p)
offset < 0x2000) ||
!chunk_is_mmapped (p) || (p->size & PREV_INUSE) ||
((((unsigned long) p - p->prev_size) & page_mask) != 0) ||
- ((sz = chunksize (p)), ((p->prev_size + sz) & page_mask) != 0))
+ ((p->prev_size + sz) & page_mask) != 0
+ )
return NULL;
- magic = MAGICBYTE (p);
- for (sz -= 1; (c = ((unsigned char *) p)[sz]) != magic; sz -= c)
+ for (sz -= 1; (c = ((unsigned char *) p)[sz]) != 0; sz -= c)
{
- if (c <= 0 || sz < (c + 2 * SIZE_SZ))
- return NULL;
+ if (sz - c <= 2 * SIZE_SZ)
+ break;
}
+ if ( c != 0 || ((unsigned char *)p)[--sz] != magic)
+ return NULL;
}
((unsigned char *) p)[sz] ^= 0xFF;
if (magic_p)