Wireshark-dev: Re: [Wireshark-dev] PSA: QString.toUtf8().constData() pattern is unsafe
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.
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.
(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.)
My guess given the limited code in your post, assuming the previous
code crashes and your version doesn't, is that the only things which
derefence the pointer given to that function are in the same scope as
the function call, therefore the realised local keeps the pointer
valid for just long enough.
That doesn't sound like a great option though, since it requires every
caller to be vary careful about that fact, and any change in scoping
potentially invisible brings the crash back into play.
Neither is it fantastic to re-malloc an object when not really
required. If the actual lifetime is much longer (you mentioned a crash
at shutdown, which lightly suggests a longer-lived problem but doesn't
necessarily prove it) 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. (Handling
the case of passing a different object of the same type is hopefully
down in the realm of perverse deliberate attempts to invoke a
crash...)
John
--
Dead stars still burn
