stack corruption in the webservice code

Linus Torvalds torvalds at linux-foundation.org
Tue Dec 10 10:18:07 UTC 2013


On Tue, Dec 10, 2013 at 8:24 AM, Lubomir I. Ivanov <neolit123 at gmail.com> wrote:
>
> this is stack corruption where calls in
> subsurfacewebservices.cpp:prepare_dives_for_divelogs() override the
> memory pointed by 'tempfile', *after* it already holds it's correct
> value (e.g. /temp/somefile.dld). the only solution for now is to give
> back 'tempfile' the sane value, as this is the actual function return.

I don't think this is a stack smash. I think it's just a nasty nasty
bug in subsurface.

You do this:

    QString tempfileQ = ...;

which creates a local variable tempfileQ. And then you do this:

    tempfile = tempfileQ.toLocal8Bit().data();

which just creates a reference to the data in that local variable.

And then you return that pointer.

That's just garbage, afaik. Why? The backing store for that data is
the QString that goes out of scope at some random time, so you're now
returning a pointer to random memory that *used* to contain the data
but who knows what it will be re-allocated for later?

You can't just look at the QString data without saving it off some way
(say, using strdup()), or alternatively making sure that the QString
in question remains live.

As far as I can tell, what happens is that you're hiding the bug by
doing that second:

        /* let's call this again */
        tempfile = tempfileQ.toLocal8Bit().data();
        return tempfile;

but that's still buggy, because you're still returning out-of-scope
data. But because you use tempfileQ at the end of the function, at
least the *first* time you used the data it will now stay around
(because it's not stale the first time since tempfileQ now ends up
having later uses, so the compiler won't kill it and do the
deconstructor for it - until later).

Basically, every time you convert from QString to a "normal" string,
you need to keep the QString around as long as that string is used. Or
do a strdup(). Or just not convert at all, and keep it as a QString,
so that the compiler will do the proper liveness analysis and keep it
around.

Too bad we don't get compiler warnings for things like this. And
apparently malloc library debugging doesn't work very well for
automatic C++ objects (since the compiler presumably just allocates
them on the stack).

There might be more of these hiding. But the minimal patch would
appear to be something like the following (totally untested and
whitespace-damaged) diff.

              Linus

---

 qt-ui/subsurfacewebservices.cpp | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/qt-ui/subsurfacewebservices.cpp b/qt-ui/subsurfacewebservices.cpp
index a8421def936f..f3d03eb602ac 100644
--- a/qt-ui/subsurfacewebservices.cpp
+++ b/qt-ui/subsurfacewebservices.cpp
@@ -115,7 +115,7 @@ static char *prepare_dives_for_divelogs(const bool selected)

        /* generate a random filename and create/open that file with zip_open */
        QString tempfileQ = QDir::tempPath() + "/import-" +
QString::number(qrand() % 99999999) + ".dld";
-       tempfile = tempfileQ.toLocal8Bit().data();
+       tempfile = strdup(tempfileQ.toLocal8Bit().data());
        zip = zip_open(tempfile, ZIP_CREATE, NULL);

        if (!zip) {
@@ -184,8 +184,6 @@ static char *prepare_dives_for_divelogs(const bool selected)
                }
        }
        zip_close(zip);
-       /* let's call this again */
-       tempfile = tempfileQ.toLocal8Bit().data();
        return tempfile;
 }


More information about the subsurface mailing list