Opened 3 years ago

Closed 16 months ago

#18145 closed defect (fixed)

OK being catched before KILL_FOCUS leads to heap-use-after-free

Reported by: ufospoke Owned by: vadz
Priority: normal Milestone:
Component: GUI-all Version: 3.1.1
Keywords: Cc: i@…
Blocked By: Blocking:
Patch: yes

Description

Using 3.1.1 on GTK (3.22.26)

In a wxDialog, I have a control that handles KILL_FOCUS to save some data.
I did not see any issue by using it but when I run the sanitizer, I
detect that KILL_FOCUS event is executed after the window is closed. I
suspect that this means that when I press Enter, 2 events are created
in that order: 1. OK button is activated, 2. the last control before
OK gets the KILL_FOCUS event.

Then,

  • event 1 is executed (OK) and the dialog is closed,
  • event 2 is executed (KILL_FOCUS) on a distroyed control.

This does not fail in practice because the memory is still there but
it's illegal and may fail some day.

Would it be possible that KILL_FOCUS be executed before OK so that
when OK is executed, we are sure there is no more event related to the
dialog?

To reproduce the issue, just run the attached program and either just press enter in the dialog (this will fail) or click on OK (this will succeed).
So activating OK by pressing Enter or by clicking the button does not give the same result in the order the events are produced.

Attachments (2)

dialog-use-after-free.cpp download (1.4 KB) - added by ufospoke 3 years ago.
sample to reproduce the issue
log.txt download (1.5 KB) - added by ufospoke 3 years ago.
sanitizer output

Download all attachments as: .zip

Change History (20)

Changed 3 years ago by ufospoke

sample to reproduce the issue

Changed 3 years ago by ufospoke

sanitizer output

comment:1 Changed 3 years ago by pcor

It appears that this is happening because an event from one window is being bound to a handler in a different window. It's not clear to me that the problem is our fault.

comment:2 Changed 3 years ago by ufospoke

  • Cc ufospoke@… added

Can you have more precision on which window, handler, event?

comment:3 follow-up: Changed 3 years ago by pcor

Your example binds wxEVT_KILL_FOCUS from a wxTextCtrl to a handler in Dialog. The handler can be called after the Dialog has been partially destructed, because the wxTextCtrl has not yet been destroyed, and can still generate events. Your Dialog should have a dtor which unbinds the handler.

comment:4 follow-up: Changed 3 years ago by vadz

In principle, we try destroying the children first exactly in order to avoid problems like this. I didn't have time to debug this yet, so I don't know why doesn't it happen here...

I also wonder if we couldn't be using wxWeakRef machinery to solve this generically.

comment:5 in reply to: ↑ 3 Changed 3 years ago by ufospoke

  • Cc ufospoke@… removed

Replying to pcor:

Your example binds wxEVT_KILL_FOCUS from a wxTextCtrl to a handler in Dialog. The handler can be called after the Dialog has been partially destructed, because the wxTextCtrl has not yet been destroyed, and can still generate events.

So you mean that the dialog can be destroyed before its child windows. However, wxWidgets claims that it is not the responsability of the user to deal with children ownership so I think the library should take care of this. Why not destroying all children of a wxWindow in the destructor before destroying the window itself?

Your Dialog should have a dtor which unbinds the handler.

How can I unbind a lambda? The Unbind function takes a function pointer as 2nd arguments.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 3 years ago by pcor

Replying to vadz:

In principle, we try destroying the children first

AFAICT, DestroyChildren() is called from wxWindow dtor, which is of course after all derived class dtors have executed, so there is no way it could be done "first".

comment:7 in reply to: ↑ 6 ; follow-up: Changed 3 years ago by vadz

Replying to pcor:

Replying to vadz:

In principle, we try destroying the children first

AFAICT, DestroyChildren() is called from wxWindow dtor, which is of course after all derived class dtors have executed, so there is no way it could be done "first".

This seems to be a similar problem to the one which requires us to call SendDestroyEvent() earlier in a few places already. Maybe we should solve it by having some StartDestroyingWindow() that would combine sending wxEVT_DESTROY and destroying children and calling this one earlier?

Or, even simpler, maybe we could just add a call to DestroyChildren() to wxWindowBase::Destroy()?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by pcor

Replying to vadz:

Or, even simpler, maybe we could just add a call to DestroyChildren() to wxWindowBase::Destroy()?

Won't help here, it's a dialog so Destroy() isn't called.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by vadz

Replying to pcor:

Replying to vadz:

Or, even simpler, maybe we could just add a call to DestroyChildren() to wxWindowBase::Destroy()?

Won't help here, it's a dialog so Destroy() isn't called.

OK, but this is just a special case that we could address by simply calling StartDestroying() or DestroyChildren() from ~wxDialog. I'm more worried about whether there could be any side effects of changing the order of destruction like this.

AFAICS it should be fine because, in principle, you could destroy all the children manually before destroying the TLW. But there could be some surprises...

comment:10 in reply to: ↑ 9 Changed 3 years ago by pcor

Replying to vadz:

OK, but this is just a special case that we could address by simply calling StartDestroying() or DestroyChildren() from ~wxDialog.

Anything we do from a dtor is technically already too late. In this case the event handler is bound in a wxDialog-derived class. The derived part has already been destructed when our dtor is called. Although I think it would fix this case, since the event is caused by something done in wxTLW dtor.

comment:11 Changed 3 years ago by ufospoke

  • Cc ufospoke@… added

However the solution to just unbind the event would work if I could unbind a lambda! Of course, I could just create a member function for this but could we think of something to unbind a lambda?

comment:12 follow-up: Changed 3 years ago by vadz

  • Component changed from wxGTK to GUI-all

I'm rather dissatisfied with the idea that you need to remember to unbind all your wxEVT_KILL_FOCUS (and what else?) handlers, this seems awfully error prone to me. If we do decide that it's the right way to deal with this problem, we really need to introduce some wxEventBinding object that would be returned by Bind() and could be passed to Unbind() and, most importantly, also some wxAutoEventBinding that would be constructible from wxEventBinding and unbind it in its dtor. The idea is that it would allow code like this:

class MyDialog : public wxDialog {
public:
    MyDialog() {
        m_focusBinding = whatever->Bind(wxEVT_KILL_FOCUS, [this](wxFocusEvent&) { ... });
    }

private:
    wxAutoEventBinding m_focusBinding;
};

and it would be safe. This is not too bad, but rather verbose and requires quite some work.

Another possibility would be to somehow generalize wxEventConnectionRef to work with lambdas, but I am not sure how to do it.

Finally, there is also a pretty hackish but simple solution of not sending wxEVT_KILL_FOCUS events to a window being destructed. This looks ugly, but should fix the problem and I can't see of any other events that would be sent during the destruction.

Any thoughts?

comment:13 in reply to: ↑ 12 Changed 3 years ago by ufospoke

  • Cc ufospoke@… removed

Replying to vadz:

I'm rather dissatisfied with the idea that you need to remember to unbind all your wxEVT_KILL_FOCUS (and what else?) handlers, this seems awfully error prone to me.

I agree

If we do decide that it's the right way to deal with this problem, we really need to introduce some wxEventBinding object that would be returned by Bind() and could be passed to Unbind()

This would of course ensure that we do not forget to unbind (like std::unique_ptr), providing we do not forget to store such object in the constructor...

About unbinding, I wonder why we cannot unbind based just on the event enum like wxEVT_KILL_FOCUS? Why do we need the address of the function?
This would allow unbinding lambdas.

Finally, there is also a pretty hackish but simple solution of not sending wxEVT_KILL_FOCUS events to a window being destructed. This looks ugly, but should fix the problem and I can't see of any other events that would be sent during the destruction.

Could we send the KILL_FOCUS earlier? before destroying any window?

comment:14 Changed 3 years ago by vadz

About unbinding, I wonder why we cannot unbind based just on the event enum like wxEVT_KILL_FOCUS? Why do we need the address of the function?

Because there can be multiple handlers for the same event and a single call to Unbind() must not disconnect all of them.

Could we send the KILL_FOCUS earlier? before destroying any window?

Not sure. In any case, the real question is whether this problem is something specific to this particular event or, at least, if it's enough to solve it in this special case, or if we need a general purpose solution.

If the answer is the former, then I think the simplest solution is to avoid sending this event to a window being destroyed. If not, we have to do something else as I don't think we can decide not to send any events to the window in this situation, this would surely break some perfectly valid uses.

comment:15 Changed 16 months ago by wangqr

  • Cc i@… added

I'm linked here from #18639. I think the issue is that event 2 is not correctly executed upon dialog close, but during dialog destroying. Destroying can happen a long time later than closing, as long as the wxDialog object is not deleted.

comment:16 Changed 16 months ago by vadz

  • Owner set to vadz
  • Patch set
  • Status changed from new to accepted

An interesting difference between wxMSW and wxGTK is that under the former the focus kill event is generated much earlier because it happens as soon as the dialog is hidden from wxDialog::EndModal(). So a simple fix for the issue with the modal dialogs would be to just mimic what wxMSW does in wxGTK and call gtk_window_set_focus(NULL) from EndModal() there too. In fact, I think it might be better to even do it whenever wxTLW is hidden, as this would be more consistent with wxMSW and it seems logical to get focus loss event when hiding the window, rather than when destroying it (possibly much later), so this is what I did in this PR.

Of course, in theory this is only a partial fix, but in practice non-modal dialogs and frames are destroyed during idle time, and not immediately, so the bug shouldn't affect them.

BTW, it's also rather weird that we have to call gtk_window_set_focus() explicitly in the first place in the dtor. This was added back in 2001 in c6ac78a61edd08ccfd0f1e5b37ee48ceb7bfa474 and it's difficult to say if it was the right thing to do. OTOH we definitely do want to get a spin event, as in #18639, when the dialog is being closed, so I don't think we can just remove this code.

comment:17 Changed 16 months ago by wangqr

I also want to point out that: if the dialog does not have the OK button, pressing Enter in spinctrl will generate spin event without losing focus. But if there is an OK button, the dialog is closed on Enter, but no spin event. Not sure if the OK button breaks something here or it's by design.

That being said, as long as the spin event is generated upon dialog close, it's fine for my use case. No matter it's generated directly from Enter key or from focus loss.

comment:18 Changed 16 months ago by Vadim Zeitlin <vadim@…>

  • Resolution set to fixed
  • Status changed from accepted to closed

In 8b90073c8/git-wxWidgets:

Send kill focus events to modal dialogs earlier in wxGTK

Ensure that the dialog is still alive when it gets the kill focus event
for its child which had focus just before the dialog was closed (or any
other events generated by this child when it detects that it's losing
focus, such as wxEVT_SPINCTRL) by resetting focus when the dialog is
being hidden and not when it's being destroyed.

This makes the events order more consistent with wxMSW but also, most
importantly, safer, as wxEVT_KILL_FOCUS handlers could previously easily
reference the fields of an already half-destroyed wxDialog-derived
object by the time they were run during wxTopLevelWindowGTK destructor
execution.

Closes #18145.

Note: See TracTickets for help on using tickets.