C++/Qt memcpy crash with QSharedMemory

Question

I have a simple function which sends strings (uri links or filepaths) to an already running instance of the application using Qt's (5.5.1) QSharedMemory class.

It seems to work correctly for most of the times, but i caught a crash log from a user, where it crashed on a memcpy. The function looks the following:

void WindowsApp::SendData( char* uri )
{
    int size = 1024;
    if (!m_SharedMemory.create(size)) {
        qDebug() << "Unable to create shared memory segment." << m_SharedMemory.error();
        return;
    }
    m_SharedMemory.lock();
    char *to = (char*)m_SharedMemory.data();
    const char *from = uri;
    memcpy(to, from, qMin(m_SharedMemory.size(), size));
    m_SharedMemory.unlock();
    QThread::sleep(10);
}

m_SharedMemory is a QSharedMemory type static member of the class.

From the log, i have seen that the string which i try to send is a simple filepath with no special characters, and not too long, only 150 chars.

What can be wrong , but so that i couldn't reproduce it with similar parameters?

Answer 1

The code has undefined behavior since you're reading past the end of the source string: you always read 1024 bytes even if the source string is e.g. 5 bytes long. UB isn't a guarantee of a crash as you've noted. Usually it will crash during an important demo. You're also not ensuring that the string will be zero-terminated if it's too long to fit in the memory segment, thus the receiver may crash if it attempts to treat the string as if it was zero-terminated.

These issues are likely due to lack of design. The contents of the memory segment imply a contract between the sender and the receiver. Both must agree on something. Let's define the contract:

1. The shared memory segment's contents are a null-terminated C string.

   This is a so-called invariant: it is always true, no matter what. This enables the reader to use C-string APIs safely without having to check first if there's a null termination.

2. A uri that's too long is replaced by an empty string.

   This is a post-condition to the writer: it implies that the writing will either place a complete URI in the memory, or an empty string.

Here's how you could fix it:

bool WindowsApp::WriteShared(const char * src, int length) {
   if (m_SharedMemory.lock()) {
      auto const dst = static_cast<char*>(m_SharedMemory.data());
      Q_ASSERT(dst);
      memcpy(dst, src, length);
      m_SharedMemory.unlock();
      return true;
   }
   return false;
}

bool WindowsApp::SendData(const char* uri)
{
   Q_ASSERT(uri);
   if (!m_SharedMemory.create(1024)) {
      qWarning() << "Unable to create shared memory segment." << m_SharedMemory.error();
      return false;
   }
   int const uriLength = strlen(uri) + 1;
   if (uriLength > m_SharedMemory.size()) {
      qWarning() << "The uri is too long.";
      if (! WriteShared("", 1))
         qWarning() << "Can't clear the memory.";
      return false;
   }
   if (! WriteShared(uri, uriLength)) {
      qWarning() << "Can't lock the shared memory segment.";
      return false;
   }
   QThread::sleep(10);
   return true;
}