Wireshark-dev: Re: [Wireshark-dev] PSA: QString.toUtf8().constData() pattern is unsafe
On Fri, Nov 28, 2014 at 4:10 PM, Peter Wu <peter@xxxxxxxxxxxxx> wrote:
> On Friday 28 November 2014 15:48:29 Evan Huus wrote:
>> I'm a bit confused - wouldn't the new instance of QByteArray simply be
>> leaked in the example code, as opposed to destructed? C++ doesn't have
>> automatic garbage collection...
>
> Why would it be leaked? There is a new QByteArray instance text_utf8
> that stays allocated until the scope is left, then its distructor is
> called.
>
> Maybe you are confused with the "new" keyword which would require you to
> add "delete" to destruct an object?
Oh, when you said "a new instance of QByteArray" I assumed that was on
the heap, but if it is on the stack then this makes perfect sense.
> Peter
>
>> On Fri, Nov 28, 2014 at 2:13 PM, Peter Wu <peter@xxxxxxxxxxxxx> wrote:
>> > Hi all,
>> >
>> > I mostly use Wireshark GTK, but just tried the Qt UI again. A recurring
>> > problem was an ASAN crash on shutdown. It turns out that there are many
>> > users of this pattern:
>> >
>> > recent_add_cfilter(NULL, currentText().toUtf8().constData());
>> >
>> > This is unsafe as currentText().toUtf8() returns a new instance of
>> > QByteArray and constData() returns a pointer to data inside that object.
>> > After returning, the data is destructed and a use-after-free condition
>> > occurs.
>> >
>> > The more correct way to do this is to use another variable to ensure
>> > that a reference is held to that QByteArray:
>> >
>> > QByteArray text_utf8 = currentText().toUtf8();
>> > recent_add_cfilter(NULL, text_utf8.constData());
>> >
>> > See also the commit message at https://code.wireshark.org/review/5528/
>> > Please avoid this pattern in the future, and watch it during reviews.
>> > --
>> > Kind regards,
>> > Peter
>> > https://lekensteyn.nl
>
