[PATCH] Fix alignment bug in Safe-Linking
Eyal Itkin
eyal.itkin@gmail.com
Mon Mar 30 19:20:10 GMT 2020
OK, will do.
I'm still not sure about how exactly I should fix the indentations. It
looks like tabs equal 8 spaces, and no more than 7 spaces should be used in
a single row. I did understand how spacing for braces and curly braces
should work. In addition, I guess that when I split up long string to
multiple lines I some how broken the indentations.
I read the style guide but couldn't come up with anything on top of what I
listed above. Is there something I'm missing? You listed at least one case
that I should fix in which I simply can't see what's wrong.
Appreciate your help,
Eyal.
On Mon, 30 Mar 2020, 21:05 Carlos O'Donell, <carlos@redhat.com> wrote:
> 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