Serializing a QHash leads to SEGFault

[gbonnema]

I have a problem with a program I am working on where serializing a QHash gets me a segfault. As the project is too much code to show, I isolated what created the problem and could reproduce the segfault.

Does anyone have a clue what I am doing wrong here? I am sorry that it's a lot of code, it's not very complex though (I am newbie at both C++ and Qt).
It runs until the statement "data2 >> datum2". After that I get a segfault.


0000 Program start.
0001 Datum1 written to buffer.
0002 Buffer about to open for reading.
0003 data2 about to initialize with buffer.
0004 data2 initialized with buffer.
0005 data2 about to read buffer into datum2.
Segmentation fault (core dumped)


This code is main.cpp.


#include <memory>
#include <iostream>
#include <exception>

#include <QCoreApplication>
#include <QBuffer>
#include <QIODevice>

#include "datum.h"

int main(int argc, char *argv[])
{
QCoreApplication a(argc, argv);

std::cout << "0000 Program start." << std::endl;

std::shared_ptr<Datum> datum = std::make_shared<Datum>();
std::shared_ptr<Thing> thing = std::make_shared<Thing>();

thing->name("name of thing");
datum->add(thing);


QBuffer buffer;

// Write datum into buffer
buffer.open(QIODevice::WriteOnly);
QDataStream data(&buffer);
data << datum;
buffer.close();
std::cout << "0001 Datum1 written to buffer." << std::endl;

// Read datum2 from buffer
std::shared_ptr<Datum> datum2;
std::cout << "0002 Buffer about to open for reading." << std::endl;
buffer.open(QIODevice::ReadOnly);
std::cout << "0003 data2 about to initialize with buffer." << std::endl;
QDataStream data2(&buffer);
std::cout << "0004 data2 initialized with buffer." << std::endl;

try {
std::cout << "0005 data2 about to read buffer into datum2." << std::endl;
data2 >> datum2;
std::cout << "0006 data2 just read buffer into datum2." << std::endl;
buffer.close();
std::cout << "0007 Datum2 read from buffer." << std::endl;
} catch (std::exception &e) {
std::cout << "0008 Error during read from buffer: " << e.what() << std::endl;
}

std::cout << "0009 Program end." << std::endl;

return a.exec();
}


Following the datum.h and datum.cpp files.


#ifndef DATUM_H
#define DATUM_H

#include <iostream>
#include <memory>

#include <QDataStream>
#include <QHash>

class Thing {

friend QDataStream &operator<< (QDataStream &ds, std::shared_ptr<const Thing>);
friend QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Thing>);

public:

QString name() const { return m_name; }
void name(const QString name) { m_name = name; }

private:
QString m_name;
};

class Datum
{

friend QDataStream &operator<< (QDataStream &ds, std::shared_ptr<const Datum>);
friend QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Datum>);

public:
Datum();
~Datum();

void add(std::shared_ptr<Thing>);

private:
QHash<QString, std::shared_ptr<Thing>> m_list;
};

QDataStream &operator<< (QDataStream &ds, std::shared_ptr<const Thing>);
QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Thing>);

QDataStream &operator<< (QDataStream &ds, std::shared_ptr<const Datum>);
QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Datum>);

#endif // DATUM_H



#include "datum.h"

Datum::Datum()
{

}

Datum::~Datum()
{

}

void Datum::add(std::shared_ptr<Thing> thing) {
m_list.insert(thing->name(), thing);
}


QDataStream &operator<< (QDataStream &ds, std::shared_ptr<const Thing> thing) {
ds << thing->m_name;
return ds;
}

QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Thing> thing) {
ds >> thing->m_name;
return ds;
}

QDataStream &operator<< (QDataStream &ds, std::shared_ptr<const Datum> datum) {
ds << datum->m_list;
return ds;
}

QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Datum> datum) {
ds >> datum->m_list;
return ds;
}

[anda_skoa]

At


data2 >> datum2;

you are trying to stream in to the shared pointer equvialent of a null pointer. I.e. there is no instance "in" datum2.

Cheers,
_

[d_stranz]

Need to add this:


std::shared_ptr<Datum> datum2 = std::make_shared<Datum>();

before the line referenced by anda_skoa.

[gbonnema]

At


data2 >> datum2;

you are trying to stream in to the shared pointer equvialent of a null pointer. I.e. there is no instance "in" datum2.

Cheers,
_


Yes, thanks you are right. Although this was not the case in the original program. After correcting it (I allocated std::make_shared<Datum>() to datum2), I still get an segfault.
After tracing using cgdb I found that the segfault is in QString::resize. I have attached a screenprint of that session, so you can see exactly what happened.

My suspicion is, that the shared_ptr in a QHash is the cause. I expected the operator>> to handle this, but I am out of my league where Qt knowledge is concerned (and maybe C++ knowledge too).

What do you think: is a QHash consisting of a
<QString, std::shared_ptr<Thing> > a valid definition for serialization?10873

Sorry, the screenprint as shown is not legible. Let me copy parts of the screenprint in text:

[CODE}
Using host libthread_db library "/lib64/libthread_db.so.1".
0000 Program start.
0001 Datum1 written to buffer.

Breakpoint 1, main (argc=1, argv=0x7fffffffdce8) at ../serialize-segfault/main.cpp:56
Missing separate debuginfos, use: debuginfo-install glib2-2.42.1-1.fc21.x86_64 libgcc-4.9.2-1.fc21.x86_64 libicu-52.1-
4.fc21.x86_64 libstdc++-4.9.2-1.fc21.x86_64 pcre-8.35-8.fc21.x86_64 zlib-1.2.8-7.fc21.x86_64
(gdb) n
(gdb) pr datum2
Ambiguous command "pr datum2": print, print-object, printf.
(gdb) print datum2
$1 = std::shared_ptr (count 1, weak 0) 0x60e760
(gdb) print datum2->m_list
$2 = {{d = 0x7ffff7c6d9a0 <QHashData::shared_null>, e = 0x7ffff7c6d9a0 <QHashData::shared_null>}}
(gdb) print datum2->m_list.keys()
Cannot evaluate function -- may be inlined
(gdb) n
0002 Buffer about to open for reading.
(gdb) n
(gdb) n
0003 data2 about to initialize with buffer.
(gdb) n
(gdb) n
0004 data2 initialized with buffer.
(gdb) n
0005 data2 about to read buffer into datum2.
(gdb) n

Program received signal SIGSEGV, Segmentation fault.
QString::resize (this=this@entry=0x0, size=size@entry=13) at tools/qstring.cpp:1585
(gdb)
[/CODE]

I hope this makes things a little more clear.

[d_stranz]

I think QString::resize() is a red herring. Your code has probably corrupted memory somewhere earlier, and it just happens to segfault at that point. If you rearranged or added some code, it would likely break somewhere else.

I think your problem is here:



QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Datum> datum)
{
ds >> datum->m_list;
return ds;
}


You may have fixed the problem with datum2 being NULL, but the std::shared_ptr< Thing > instances that are the values stored for the hash are still NULL. When your serialization hits this:



QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Thing> thing)
{
ds >> thing->m_name;
return ds;
}

it blows up because the std::shared_ptr< Thing > is uninitialized and contains a NULL Thing instance. Possibly you can check to see if "thing" is NULL, and if so, call std::make_shared< Thing >() at that point:



QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Thing> thing)
{
if ( !thing )
thing = std::make_shared< Thing >();
ds >> thing->m_name;
return ds;
}

You may need to pass in the "thing" argument as a reference, (std::shared_ptr< Thing > & thing) but I don't know the semantics of std::shared_ptr in this use.

EDIT:

Actually, on thinking about it, the QString::resize() probably was actually where things went wrong, because the QString it was trying to resize is the one that is contained in the NULL Thing instance - it's trying to resize something that doesn't exist. That was triggered by ds >> thing->m_name, when "thing" is NULL.

[gbonnema]

Thank you very much! Not only do I now see what went wrong, but also I understand better how to approach this type of problem.
In summary:

1. My operator>> for Thing needs updating to cater for nullptr.
2. The shared_ptr may or may not need to be a reference.

Thanks again: I appreciate your help!

EDIT: Just wanted to point to http://stackoverflow.com/questions/327573/c-passing-references-to-stdshared-ptr-or-boostshared-ptr. It contains an excellent discussion of point 2, although to me on first reading it is still not clear whether using a shared_ptr as a reference is a wise thing to do.

EDIT2: On second thought, I suppose referencing a shared_ptr probably defeats the goal of a shared_ptr which is to do bookkeeping on how many users of a storage area there are and only delete when the last one releases the pointer. If only 2 users are left and one uses a reference, then as soon as the other one releases the pointer the memory area will be deleted. So, probably if a shared_ptr is necessary, one should not want to reference it.

EDIT3: The last code example you give the decision seems to point in the opposite direction: we *have* to use reference in this case.



QDataStream &operator>> (QDataStream &ds, std::shared_ptr<Thing> thing) {
if (thing == nullptr) {
thing = std::make_shared<Thing>();
}
ds >> thing->m_name;
return ds;
}


What happens here is that a temporary gets created for the param thing because it is by value. Then the resulting address of make_shared is copied into the temporary thing.
When the function goes out of scope the question is: will the memory area that make_shared created still exist by the time thing gets inserted into the Datum container or will it have been deleted by that time?

I fear that a reference to the shared_ptr is a must in this case. I don't see any alternative. Any comments?

[anda_skoa]

If you look at the documentation, the "read" function/operator always uses a reference for the data element:
http://doc.qt.io/qt-5/qdatastream.html#reading-and-writing-other-qt-classes

Cheers,
_

[gbonnema]

If you look at the documentation, the "read" function/operator always uses a reference for the data element:
http://doc.qt.io/qt-5/qdatastream.html#reading-and-writing-other-qt-classes

Cheers,
_

Well, actually, I completely get why for a normal object this should always be a reference both in reading and in writing for operator>> and operator<< overrides.
Also, thanks to your comments, I see that operator>> should always use a reference to shared_ptr as well, because one is modifying memory that one sometimes allocates through this variable. (see my previous comment).

However, when reading the shared_ptr variable with operator<<, I am not convinced it should be a reference to shared_ptr. The reason is, that if you use a reference, then the shared_ptr will not increment its ref count.
Therefore during the call to the << function if the other user deletes the pointer (i.e. goes out of scope) .... Wait, this could only happen in a multi threaded environment where the caller runs async from the << function.

ok, I suspect with the << function you could use reference to prevent copying of the pointer when no concurrency is in play, but using parameter by value should work too. For the reading side of the equation I am not convinced for either case: by reference or by value.
But, in the face of concurrency, by value might be the safest bet as the ref count gets incremented so the memory area remains valid no matter what.

hmm, conclusion:

1. For operator>> (i.e. write) definitely reference as you are potientially modifying the address for a nullptr
2. For operator<< (i.e. read) I do not know which is best in the case of shared_ptr.

Thanks for your comments. It has raised some pretty interesting points.

[anda_skoa]

But, in the face of concurrency, by value might be the safest bet as the ref count gets incremented so the memory area remains valid no matter what.

The caller of the operator still has a valid reference, the counter cannot drop to 0.

Cheers,
_

[gbonnema]

The caller of the operator still has a valid reference, the counter cannot drop to 0.

Cheers,
_

True, but only if called synchronously. In a chain of calls where only the first caller has a shared_ptr and the called functions all have references to shared_ptr, imagine the first function being called asynchronously.
In that case, if the caller ends before the chain of calls ends, then they all refer to memory that was deleted when the caller ended.

Furthermore the writer of the operator override can not know how it is being called. So, unless you control all callers, using a reference to shared_ptr, holds a risk of dangling pointer.
If you are writing a class that could be used in multiple applications, you can not assume anything.

It is this case that I meant. Passing by value is safest because it prevents the memory from being deleted. Although you could leave the concurrent case as "behaviour undefined".

[anda_skoa]

What kind of asynchronous call mechanism do you have in mind that would not trigger a copy but work with a non-shared pointer reference going out of scope?

Cheers,
_