Wireshark-dev: Re: [Wireshark-dev] PSA: QString.toUtf8().constData() pattern is unsafe
May be add a check (in checkAPI) to detect this mistake ?
On Sat, Nov 29, 2014 at 1:38 AM, John Sullivan
<jsethdev@xxxxxxxxxxxxxxxxxxxx> 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.
>
> 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
>
> ___________________________________________________________________________
> Sent via: Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
> Archives: http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
> mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe
