Temporary string data in Qt

[Henrik Brautaset Aronsen]

Very nice, Alberto!

You should post this as a patch, and just add everything in this mail in
the commit comment.

Henrik


On Fri, May 24, 2013 at 11:39 AM, Alberto Mardegan <
mardy at users.sourceforge.net> wrote:

> 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!
> _______________________________________________
> subsurface mailing list
> subsurface at hohndel.org
> http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.hohndel.org/pipermail/subsurface/attachments/20130524/bfb91600/attachment.html>