May be add a check (in checkAPI) to detect this mistake ?
On Sat, Nov 29, 2014 at 1:38 AM, John Sullivan <[email protected]> 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 <[email protected]> > Archives: http://www.wireshark.org/lists/wireshark-dev > Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev > mailto:[email protected]?subject=unsubscribe ___________________________________________________________________________ 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
