Wireshark mailing list archives
Re: PSA: QString.toUtf8().constData() pattern is unsafe
From: Alexis La Goutte <alexis.lagoutte () gmail com>
Date: Sat, 29 Nov 2014 11:25:15 +0100
May be add a check (in checkAPI) to detect this mistake ? On Sat, Nov 29, 2014 at 1:38 AM, John Sullivan <jsethdev () kanargh force9 co uk> 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 () 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
___________________________________________________________________________ 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
Current thread:
- PSA: QString.toUtf8().constData() pattern is unsafe Peter Wu (Nov 28)
- Re: PSA: QString.toUtf8().constData() pattern is unsafe Evan Huus (Nov 28)
- Re: PSA: QString.toUtf8().constData() pattern is unsafe Peter Wu (Nov 28)
- Re: PSA: QString.toUtf8().constData() pattern is unsafe Evan Huus (Nov 28)
- Re: PSA: QString.toUtf8().constData() pattern is unsafe Peter Wu (Nov 28)
- Re: PSA: QString.toUtf8().constData() pattern is unsafe John Sullivan (Nov 28)
- Re: PSA: QString.toUtf8().constData() pattern is unsafe Alexis La Goutte (Nov 29)
- Re: PSA: QString.toUtf8().constData() pattern is unsafe Peter Wu (Nov 29)
- Re: PSA: QString.toUtf8().constData() pattern is unsafe Evan Huus (Nov 28)