Wireshark mailing list archives
Re: PSA: QString.toUtf8().constData() pattern is unsafe
From: Peter Wu <peter () lekensteyn nl>
Date: Sat, 29 Nov 2014 19:09:09 +0100
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
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)