Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-59804

qReallocAligned does not work correctly in certain situations

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: P2: Important
    • Resolution: Done
    • Affects Version/s: 5.6.2, 5.8.0
    • Fix Version/s: 5.9.0
    • Component/s: Core: Other
    • Labels:
      None
    • Commits:
      6a83f9aa98760d0ed1ab4c216a28a10862e86f00

      Description

      http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/global/qmalloc.cpp

      void *qReallocAligned(void *oldptr, size_t newsize, size_t oldsize, size_t alignment)
      {
          // fake an aligned allocation
          Q_UNUSED(oldsize);
      
          void *actualptr = oldptr ? static_cast<void **>(oldptr)[-1] : 0;
          if (alignment <= sizeof(void*)) {
              // special, fast case
              void **newptr = static_cast<void **>(realloc(actualptr, newsize + sizeof(void*)));
              if (!newptr)
                  return 0;
              if (newptr == actualptr) {
                  // realloc succeeded without reallocating
                  return oldptr;
              }
      
              *newptr = newptr;
              return newptr + 1;
          }
      
          // malloc returns pointers aligned at least at sizeof(size_t) boundaries
          // but usually more (8- or 16-byte boundaries).
          // So we overallocate by alignment-sizeof(size_t) bytes, so we're guaranteed to find a
          // somewhere within the first alignment-sizeof(size_t) that is properly aligned.
      
          // However, we need to store the actual pointer, so we need to allocate actually size +
          // alignment anyway.
      
          void *real = realloc(actualptr, newsize + alignment);
          if (!real)
              return 0;
      
          quintptr faked = reinterpret_cast<quintptr>(real) + alignment;
          faked &= ~(alignment - 1);
      
          void **faked_ptr = reinterpret_cast<void **>(faked);
      
          // now save the value of the real pointer at faked-sizeof(void*)
          // by construction, alignment > sizeof(void*) and is a power of 2, so
          // faked-sizeof(void*) is properly aligned for a pointer
          faked_ptr[-1] = real;
      
          return faked_ptr;
      }
      
      int main(int argc, char* argv[])
      {
          size_t s=8;
          volatile char*p=(char*)qReallocAligned(0,s,0,1);
          volatile uintptr_t&ph=*((volatile uintptr_t*)&p);
          memcpy((void*)p,"fuck",5);
          volatile char*pn=(char*)qReallocAligned((void*)p,s,0,16);
          volatile uintptr_t&pnh=*((volatile uintptr_t*)&pn);
          if(pn)
              p=pn;
          bool ok=p[0]=='f';
          if(ok)
              printf("\n%s\n",p);
          else
              printf("\ngotcha\n");
          qFreeAligned((void*)p);
          return ok?0:1;
      }
      

      alignment is changed in second qReallocAligned invocation, so faked_ptr also changes(note: it MAY stay the same, but this is not guaranteed).
      the result is old data is in alignment block now, and returned pointer points to uninitialized data.
      you can use malloc+memcpy instead of realloc, but there is no way to memcpy reallocated block, unless you do it byte-by-byte end-to-beginning OR beginning-to-end, depending on alignment differences.
      and "special, fast case" is NOT fast. for one, realloc is much more heavy than this wrapper, and anyway, you add comparison to avoid bitwise and. it's unnecessary.

        Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            People

            • Assignee:
              thiago Thiago Macieira
              Reporter:
              madmonkey madmonkey
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Gerrit Reviews

                There are no open Gerrit changes