Re: PSA: QString.toUtf8().constData() pattern is unsafe

[Peter Wu <peter () lekensteyn nl>]

On Saturday 29 November 2014 00:38:34 John Sullivan wrote:
> On Friday, November 28, 2014, 7:13:26 PM, Peter Wu 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());
>
> This is not necessarily a problem with the calling code, and (without
> having personally actually checked either the calling or the called
> code) may not be solvable with just that change.
>
> The temporary is guaranteed to live until the end of the full
> expression.
>
> Which means that during the execution of recent_add_cfilter, the
> pointer is guaranteed to be valid.

Ah cool, I did not know this. I assumed that the pointer became invalid
after the (sub)expression is evaluated rather than after the statement.

> What isn't valid is assuming that same pointer is valid after that
> point. The terminating semicolon. So if recent_add_cfilter implies
> that the data will be needed after the point at which
> recent_add_cfilter returns, it must either copy the data, or if the
> data is reference counted/countable, add a reference to it.

This is exactly what I encountered, and due to the previous assumption I
made, I extended it to other uses of the pattern (including the provided
example).

What actually crashed was the code that saves Recent files (because it
stores a pointer to a const char*), and a UAT change handler
(Preferences -> Protocols -> SSL -> Edit RSA keys, "Add" and try to type
something --> instant ASAN crash).

> (A const& assigned to a temporary will keep the temporary alive to the
> end of the scope of the const&, so in any case it is a good idea in
> these cases to make the local a const& type. If the called function
> returns a real object, it'll go in to a temporary and be no different.
> If it returns a & to an existing object, then the call*ing* code
> doesn't need to make a whole new copy of the full object and is
> therefore much more efficient.)

(Thanks for explaining, I haven't touched my C++ book for a long time)

> Neither is it fantastic to re-malloc an object when not really
> required. If the actual lifetime is much longer then re-allocation is
> fine, but if it is genuinely locally scoped then a better solution is
> to refactor any called functions within that scope so they all take
> references to the operated on object, then it becomes physically
> impossible to call them without still having a live object of the
> appropriate type.

All users do not modify the char pointer, and if they needed to store
them, they do duplicate the memory.

I will amend the patch. Thanks again for clarifying C++ behavior!
-- 
Kind regards,
Peter
https://lekensteyn.nl

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe