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?

`memcpy(to, from, qMin(m_SharedMemory.size(), size));` - the same size 2 times. Must be `strlen(uri)` instead of `size`. — KonstantinL, Apr 13 '17 at 11:44
Btw, you should NEVER lock and unlock like that! Espectially when you use such a volatile operation such as `memcpy`. Learn how to use a [`lock_guard`](http://en.cppreference.com/w/cpp/thread/lock_guard) to make your multithreaded code exception safe, even if you have to build your own lock_guard. Besides, you should consider `std::copy`, which is less dangerous compared to `memcpy`, which is C basically, not C++. — The Quantum Physicist, Apr 13 '17 at 11:45
I agree partly with KonstantinL (except I recommend `strlen(uri) + 1`) and The Quantum Physicist but I believe both arguments don't explain your watched behavior. I studied the [Shared Memory Example](http://doc.qt.io/qt-5/qtcore-ipc-sharedmemory-example.html) and realized your code resembles it partly. So, does the receiver side (you did not show) use `lock()` and `unlock()` also? IMHO sporadic errors in multi-threading are an indicator that (mutex) locking is not done properly. Terrible, to find the resp. piece of code - you have to check everything involved... — Scheff's Cat, Apr 13 '17 at 12:02
heh, didn't catched the size mistake yet, i will fix that, but i don't think that it has anything to do with the problem. yes, receiver side also uses lock. but i feel like lock_guard would be overkill for a simple two sided send and receive. — Hajdu Gábor, Apr 13 '17 at 12:05
To make multi-threading issues reproducable, is hard if not impossible. (Mutli-threading introduces non-deterministic behavior by definition.) You can try to "force" it (in the real sense of word). So, you could try to lower the number in `QThread::sleep()` (e.g. 0). This will probably slow down everything and produce some inconvenient cooler noise but increases the chances to get a collision soon... — Scheff's Cat, Apr 13 '17 at 12:06
yes, it is quite hard to debug, execptionally, because as it is not shared in a simple application, but an application opened twice, and the sender application closes immediately after send. — Hajdu Gábor, Apr 13 '17 at 12:10
@HajduGábor Ok, in this case... Did you notice the `attach()`/`detach()` "magic" also? I didn't know `QSharedMemory` until today but it sounds like they use memory mapped files which we use on Windows for IPC with shared memory... — Scheff's Cat, Apr 13 '17 at 12:12
yes, create() automatically does attach, and on the sender side, there is an attach()/detach() pair used. — Hajdu Gábor, Apr 13 '17 at 12:21
If the sender exits to early, could it be that the receiver is not able to attach before the sender auto-detachs upon exit. (Just a theory...) Another idea: What happens when sender creates a new size while receiver is not yet detached (from previous attempt to receive)? (These things should be reproducable in the debugger with a break-point at the right place or a pause forced with `cin`.) — Scheff's Cat, Apr 13 '17 at 12:30
@Scheff i checked it, early exit simply causes that i can't read on the receiving side. Stopping before detach, and starting a new one also didn't cause crash. — Hajdu Gábor, Apr 13 '17 at 13:41
@HajduGábor What if the user did something unexpected (what he likely didn't report)? E.g. he started your application twice. How is the name for the `QSharedMemory` chosen? What happens if 2nd sender `create()`s the shared memory while 1st sender has (a) `lock()`ed it or (b) not yet `lock()`ed it? — Scheff's Cat, Apr 13 '17 at 13:49
@Scheff He couldn't have had it opened twice because the only possible thing that could happen on a second opening is a sending. Before this, for the previous check, i checked it before, and between the locks, and it didn't crash. I also made some maniac sending barrage with multiple openings, but it didn't crash — Hajdu Gábor, Apr 13 '17 at 14:07
@HajduGábor So, I'm rather out of ideas. Of course, there is still another possible explanation: Memory is corrupted by anything else... — Scheff's Cat, Apr 13 '17 at 14:12
@HajduGábor A collaborator gave me another hint: You should check the result of `QSharedMemory::data()`. We do not know whether this (c|sh)ould fail but a pointer should be checked. `uri` is another thing which could be wrong. On Windows: there is additional info in the exception code where you can see whether read (`from`) or write (`to`) caused the crash in `memcpy()`. This might provide a hint also. — Scheff's Cat, Apr 13 '17 at 14:45
Re the size mistake "i don't think that it has anything to do with the problem" It has everything to do with the problem, because you have blatant undefined behavior in your code. You can't know if the entire 1024 bytes starting at the `uri` are mapped in; recall that `uri` is typically a virtual memory address, not a physical address. When there's no mapping, you'll get a segmentation fault. You can't even begin to debug it while leaving that "nugget" in. — Kuba hasn't forgotten Monica, Apr 13 '17 at 15:21
While I read the answer of Kuba Ober - another idea came in mind: Due to `memcpy(to, from, qMin(m_SharedMemory.size(), size));` `memcpy()` may read-access bytes behind the end of `uri` which may or may not end up in a seg. fault. Whether this crashes depends on random things in internal memory management. This could be an explanation for the sporadic crash. If it does not crash you will have some "trash" in shared memory (behind the zero termination) which you probably never realized. — Scheff's Cat, Apr 13 '17 at 15:25

score 2 · Accepted Answer · edited Jun 20 '20 at 09:12

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:

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.
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;
}

C++/Qt memcpy crash with QSharedMemory

1 Answers1