[PATCH] Fix alignment bug in Safe-Linking
Carlos O'Donell
carlos@redhat.com
Mon Mar 30 18:05:15 GMT 2020
On 3/30/20 1:18 PM, Eyal Itkin via Libc-alpha wrote:
> 1. Alignment checks should be performed on the user's buffer and NOT
> on the mchunkptr as was done before. This caused bugs in 32 bit
> versions.
> 2. Improved the testing and fixed the check for the tcache case.
> 3. Removed unneeded '\' chars from end of lines.
Please split this into 2 patches.
1 - typo fixes, with Andreas' change added.
2 - Bug fix.
I'll review both and retest.
> This fixes the bugs on 32 bit archs as were found by Andreas.
> I confused the alignments and initially checked the mchunkptr instead
> of the user's buffer. On 64 bit binaries they are both aligned to
> 0x10, but on 32 bit binaries malloc still expects alignment of 0x10,
> and the delta between the user's buffer to mchunkptr was 8 bytes and
> this is why it failed the checks. Now I checked it on a 32 bit intel
> machine and a 64 bit intel machine, and they both pass the tests.
>
> It is important to note that the tcache works on the user's buffer,
> while the fastbins use the mchunkptr type. This means I had to adjust
> the alignment checks accordingly to use aligned_OK() and
> misaligned_chunk().
>
> Now that this is over, I will format my tests to match glibc's tests
> and submit them as well.
>
> From c86dbe3e2cc789068f27ee8fad7a7af3795cd1d1 Mon Sep 17 00:00:00 2001
> From: Eyal Itkin <eyalit@checkpoint.com>
> Date: Mon, 30 Mar 2020 13:01:59 -0400
> Subject: [PATCH] Fix alignment bug in Safe-Linking
>
> 1. Alignment checks should be performed on the user's buffer and
> NOT on the mchunkptr as was done before. This caused bugs in 32 bit
> versions.
>
> 2. Improved the testing and fixed the check for the tcache case.
>
> 3. Removed unneeded '\' chars from end of lines.
> ---
> malloc/malloc.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 1282863681..50bd727f38 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2169,8 +2169,8 @@ do_check_malloc_state (mstate av)
>
> while (p != 0)
> {
> - if (__glibc_unlikely (!aligned_OK (p)))
> - malloc_printerr ("do_check_malloc_state(): " \
> + if (__glibc_unlikely (misaligned_chunk (p)))
> + malloc_printerr ("do_check_malloc_state(): "
OK. Fixed formatting.
> "unaligned fastbin chunk detected");
> /* each chunk claims to be inuse */
> do_check_inuse_chunk (av, p);
> @@ -2949,11 +2949,11 @@ static __always_inline void *
> tcache_get (size_t tc_idx)
> {
> tcache_entry *e = tcache->entries[tc_idx];
> + if (__glibc_unlikely (!aligned_OK (e)))
> + malloc_printerr ("malloc(): unaligned tcache chunk detected");
> tcache->entries[tc_idx] = REVEAL_PTR (e->next);
> --(tcache->counts[tc_idx]);
> e->key = NULL;
> - if (__glibc_unlikely (!aligned_OK (e)))
> - malloc_printerr ("malloc(): unaligned tcache chunk detected");
OK. Moved test earlier since the revealed pointer is not needed.
> return (void *) e;
> }
>
> @@ -2978,7 +2978,7 @@ tcache_thread_shutdown (void)
> {
> tcache_entry *e = tcache_tmp->entries[i];
> if (__glibc_unlikely (!aligned_OK (e)))
> - malloc_printerr ("tcache_thread_shutdown(): " \
> + malloc_printerr ("tcache_thread_shutdown(): "
> "unaligned tcache chunk detected");
OK. Fixed formatting.
> tcache_tmp->entries[i] = REVEAL_PTR (e->next);
> __libc_free (e);
> @@ -3591,7 +3591,7 @@ _int_malloc (mstate av, size_t bytes)
> if (victim == NULL) \
> break; \
> pp = REVEAL_PTR (victim->fd); \
> - if (__glibc_unlikely (!aligned_OK (pp))) \
> + if (__glibc_unlikely (pp != NULL && misaligned_chunk (pp))) \
OK. Using misaligned_chunk.
> malloc_printerr ("malloc(): unaligned fastbin chunk detected"); \
> } \
> while ((pp = catomic_compare_and_exchange_val_acq (fb, pp, victim)) \
> @@ -3606,8 +3606,8 @@ _int_malloc (mstate av, size_t bytes)
>
> if (victim != NULL)
> {
> - if (__glibc_unlikely (!aligned_OK (victim)))
> - malloc_printerr ("malloc(): unaligned fastbin chunk detected");
> + if (__glibc_unlikely (misaligned_chunk (victim)))
> + malloc_printerr ("malloc(): unaligned fastbin chunk detected 2");
OK. Using miasligned_chunk.
>
> if (SINGLE_THREAD_P)
> *fb = REVEAL_PTR (victim->fd);
> @@ -3631,8 +3631,8 @@ _int_malloc (mstate av, size_t bytes)
> while (tcache->counts[tc_idx] < mp_.tcache_count
> && (tc_victim = *fb) != NULL)
> {
> - if (__glibc_unlikely (!aligned_OK (tc_victim)))
> - malloc_printerr ("malloc(): unaligned fastbin chunk detected");
> + if (__glibc_unlikely (misaligned_chunk (tc_victim)))
> + malloc_printerr ("malloc(): unaligned fastbin chunk detected 3");
OK. Using misaligned_chunk. I like the # to detect which case (always my preference).
> if (SINGLE_THREAD_P)
> *fb = REVEAL_PTR (tc_victim->fd);
> else
Misses one extra indentation issue in _int_free highlighted by Andreas.
@@ -4225,14 +4225,14 @@ _int_free (mstate av, mchunkptr p, int have_lock)
for (tmp = tcache->entries[tc_idx];
tmp;
tmp = REVEAL_PTR (tmp->next))
- {
- if (__glibc_unlikely (!aligned_OK (tmp)))
- malloc_printerr ("free(): unaligned chunk detected in tcache 2");
- if (tmp == e)
- malloc_printerr ("free(): double free detected in tcache 2");
- /* If we get here, it was a coincidence. We've wasted a
- few cycles, but don't abort. */
- }
+ {
+ if (__glibc_unlikely (!aligned_OK (tmp)))
+ malloc_printerr ("free(): unaligned chunk detected in tcache 2");
+ if (tmp == e)
+ malloc_printerr ("free(): double free detected in tcache 2");
+ /* If we get here, it was a coincidence. We've wasted a
+ few cycles, but don't abort. */
+ }
}
if (tcache->counts[tc_idx] < mp_.tcache_count)
> @@ -4505,8 +4505,8 @@ static void malloc_consolidate(mstate av)
> if (p != 0) {
> do {
> {
> - if (__glibc_unlikely (!aligned_OK (p)))
> - malloc_printerr ("malloc_consolidate(): " \
> + if (__glibc_unlikely (misaligned_chunk (p)))
> + malloc_printerr ("malloc_consolidate(): "
> "unaligned fastbin chunk detected");
OK. Fix indentation.
>
> unsigned int idx = fastbin_index (chunksize (p));
> @@ -4937,8 +4937,8 @@ int_mallinfo (mstate av, struct mallinfo *m)
> p != 0;
> p = REVEAL_PTR (p->fd))
> {
> - if (__glibc_unlikely (!aligned_OK (p)))
> - malloc_printerr ("int_mallinfo(): " \
> + if (__glibc_unlikely (misaligned_chunk (p)))
> + malloc_printerr ("int_mallinfo(): "
> "unaligned fastbin chunk detected");
OK. Fix indentation.
> ++nfastblocks;
> fastavail += chunksize (p);
> @@ -5479,8 +5479,8 @@ __malloc_info (int options, FILE *fp)
>
> while (p != NULL)
> {
> - if (__glibc_unlikely (!aligned_OK (p)))
> - malloc_printerr ("__malloc_info(): " \
> + if (__glibc_unlikely (misaligned_chunk (p)))
> + malloc_printerr ("__malloc_info(): "
> "unaligned fastbin chunk detected");
OK. Fix indentation.
> ++nthissize;
> p = REVEAL_PTR (p->fd);
> --
> 2.20.1
>
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list