Temporary string data in Qt

[Alberto Mardegan]

Hi all,
  I had a look at the last commits in master and thought that people
which are new to Qt might appreciate this review:

=======
@@ -360,7 +373,7 @@ bool WeightModel::setData(const QModelIndex& index,
const QVariant& value, int r
        switch(index.column()) {
        case TYPE:
                if (!value.isNull()) {
-                       char *text= value.toByteArray().data();
+                       char *text =
strdup(value.toString().toUtf8().data());
                        if (!ws->description || strcmp(ws->description,
text)) {
                                ws->description = strdup(text);
                                mark_divelist_changed(TRUE);
=======

There's a small mistake introduced here, that is a memory leak: the data
pointed by "text" is not freed ("strdup(text)" is probably freed later
on, but not the original data pointed by "test").

I guess that this change was added to work around a random crash. So,
here's an explanation of the reason of why the previous code could
crash: toByteArray(), as well as others "toSomething()" methods, returns
a new object which the compiler allocates on the stack. The compiler
will consider it a temporary data, and destroy it on the next line. So,
when you do

  char *text= value.toByteArray().data(); // line 1
  if (strcmp(description, text)) {        // line 2

the compiler creates a QByteArray on line 1, calls ::data() on it, which
returns a valid char *, and assigns its value to "text". So far, so
good. But before jumping to line 2, the compiler destroys the temporary
QByteArray, and this will in turn invoke the QByteArray destructor,
which will destroy the internal data. The result is that on line 2,
"text" will point to some memory which has already been freed.

One solution is to use a temporary variable:

========
@@ -373,7 +374,8 @@ bool WeightModel::setData(const QModelIndex& index,
const QVariant& value, int r
        switch(index.column()) {
        case TYPE:
                if (!value.isNull()) {
-                       char *text =
strdup(value.toString().toUtf8().data());
+                       QByteArray ba = value.toString().toUtf8();
+                       const char *text = ba.constData();
                        if (!ws->description || strcmp(ws->description,
text)) {
                                ws->description = strdup(text);
                                mark_divelist_changed(TRUE);
========

in this way, the temporary QByteArray returned by QString::toUtf8() is
copied into a local variable: the compiler will still destroy the
temporary QByteArray it created, but (thanks to the reference-counted
data sharing built in QByteArray) now the destructor will see that the
data is referenced by another instance of QByteArray (the local variable
"ba") and will not free the internal data.
In the code above, the internal data will be available until "ba" is
destroyed, which will happen at the end of the {} block where it is defined.

Please note that while you use the data in the same line, you don't need
to worry about this issue. In fact,

  text = strdup(value.toString().toUtf8().data());

works just fine (my complaint was on the memory leak, an on the fact
that this copy can be avoided).

Sorry for the long mail. :-)

Ciao,
  Alberto

-- 
http://blog.mardy.it <- geek in un lingua international!