stack corruption in the webservice code

Lubomir I. Ivanov neolit123 at gmail.com
Tue Dec 10 12:25:45 UTC 2013


On 10 December 2013 20:18, Linus Torvalds <torvalds at linux-foundation.org> wrote:
> 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);
>

ok, this makes sense.

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

i will improve this a little and send another patch.
we then have to free that memory block for the string as well (if not NULL).

lubomir
--


More information about the subsurface mailing list