Message ID | dd545f772bb48cba9ddf98148e81d0a1362a074e.1614874816.git.szabolcs.nagy@arm.com |
---|---|
State | New |
Headers | show |
Series | memory tagging improvements | expand |
The 03/04/2021 19:15, DJ Delorie wrote: > Szabolcs Nagy <szabolcs.nagy@arm.com> writes: > > diff --git a/malloc/malloc.c b/malloc/malloc.c > > index 1f4bbd8edf..10ea6aa441 100644 > > --- a/malloc/malloc.c > > +++ b/malloc/malloc.c > > @@ -3446,7 +3446,9 @@ __libc_realloc (void *oldmem, size_t bytes) > > newp = __libc_malloc (bytes); > > if (newp != NULL) > > { > > - memcpy (newp, oldmem, oldsize - SIZE_SZ); > > > + size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ; > > I think this is semantically wrong, because the chunk size > (mptr->mchunk_size) does not include the mchunk_prev_size that's > accounted for in CHUNK_HDR_SZ. I suspect the problem is that > CHUNK_AVAILABLE_SIZE is wrong, in that it adds SIZE_SZ in the non-tagged > case, and shouldn't, or that it's defined (or named) wrong. > > chunksize(p) is the difference between this chunk and the corresponding > address in the next chunk. i.e. it's prev_ptr to prev_ptr, or > user-bytes to user-bytes. > > A "chunk pointer" does NOT point to the beginning of the chunk, but to > the prev_ptr in the PREVIOUS chunk. So CHUNK_HDR_SZ is the offset from > a chunk pointer to the user data, but it is NOT the difference between > the chunk size and the user data size. Using CHUNK_HDR_SZ in any > user-data-size computations is suspect logic. > > That the resulting value happens to be correct is irrelevent here, > although I suspect it will be off by a word when tagging is enabled, and > not memcpy enough data, if the prev_ptr word is still part of the "user > data" when tagging is enabled. it seems CHUNK_AVAILABLE_SIZE is defined as (memory owned by the user) + CHUNK_HDR_SZ and it should work on mmaped and normal chunks with or without tagging. so by this definition i think the change is right, but the CHUNK_AVAILABLE_SIZE may not have the most useful definition. i can change this macro to be more meaningful, e.g.: CHUNK_USER_SIZE(chunk): memory owned by the user in chunk. i.e. the interval that user code may access in chunk p is [ chunk2mem(p), chunk2mem(p) + CHUNK_USER_SIZE(p) ) with tagging on aarch64 (granule = 2*size_t) this does not include the prev_ptr word at the end. I can refactor the code using this macro, or let me know if you have a different preference (and if it should be backported with this bug fix or have it as a follow up change on master).
diff --git a/malloc/malloc.c b/malloc/malloc.c index 1f4bbd8edf..10ea6aa441 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -3446,7 +3446,9 @@ __libc_realloc (void *oldmem, size_t bytes) newp = __libc_malloc (bytes); if (newp != NULL) { - memcpy (newp, oldmem, oldsize - SIZE_SZ); + size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ; + memcpy (newp, oldmem, sz); + (void) TAG_REGION (chunk2rawmem (oldp), sz); _int_free (ar_ptr, oldp, 0); } } @@ -4850,10 +4852,10 @@ _int_realloc(mstate av, mchunkptr oldp, INTERNAL_SIZE_T oldsize, else { void *oldmem = chunk2mem (oldp); + size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ; newmem = TAG_NEW_USABLE (newmem); - memcpy (newmem, oldmem, - CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ); - (void) TAG_REGION (chunk2rawmem (oldp), oldsize); + memcpy (newmem, oldmem, sz); + (void) TAG_REGION (chunk2rawmem (oldp), sz); _int_free (av, oldp, 1); check_inuse_chunk (av, newp); return chunk2mem (newp);