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 <[email protected]> Archives: http://www.wireshark.org/lists/wireshark-dev Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev mailto:[email protected]?subject=unsubscribe
