Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

selection menu crash (use after free) #147

Closed
Xeverous opened this issue Jun 12, 2020 · 14 comments
Closed

selection menu crash (use after free) #147

Xeverous opened this issue Jun 12, 2020 · 14 comments

Comments

@Xeverous
Copy link
Contributor

MCVE:

#include <elements.hpp>

using namespace cycfi::elements;

// Main window background color
auto constexpr bkd_color = rgba(35, 35, 37, 255);
auto background = box(bkd_color);

struct user_state
{
        std::string menu_item;
};

template <typename Subject>
proxy<cycfi::remove_cvref_t<Subject>>
make_proxy(Subject&& subject)
{
        return { std::forward<Subject>(subject) };
}

auto make_button(std::string text, std::function<void(bool)> on_click)
{
        auto but = button(std::move(text));
        but.on_click = std::move(on_click);
        return but;
}

auto make_menu(const std::vector<std::string>& names, user_state& state)
{
        return selection_menu(
                [&state](std::string_view selected)
                { state.menu_item = selected; },
                names).first;
}

auto make_elements(view& view, user_state& state)
{
        auto m = share(make_proxy(make_menu({ "One", "Two" }, state)));
        auto b = make_button("chnage menu", [m, &state](bool){ m->subject(make_menu({"One", "Two", "Three", "Four"}, state)); });
        return align_top(vtile(hold(m), std::move(b)));
}

int main(int argc, char* argv[])
{
   app _app(argc, argv);
   window _win(_app.name());
   _win.on_close = [&_app]() { _app.stop(); };

   user_state state;

   view view_(_win);

   view_.content(
      make_elements(view_, state),
      background
   );

   _app.run();
   return 0;
}
==14711==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000001850 at pc 0x7fe062332867 bp 0x7ffcca9f4e90 sp 0x7ffcca9f4638
READ of size 3 at 0x606000001850 thread T0
    #0 0x7fe062332866  (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x3f866)
    #1 0x558b2d46e7ff in std::char_traits<char>::copy(char*, char const*, unsigned long) /usr/include/c++/8/bits/char_traits.h:352
    #2 0x558b2d49750a in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_S_copy(char*, char const*, unsigned long) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xeb50a)
    #3 0x558b2d4a2125 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_replace(unsigned long, unsigned long, char const*, unsigned long) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xf6125)
    #4 0x558b2d4a4e76 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::assign(char const*, unsigned long) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xf8e76)
    #5 0x558b2d49c5ff in std::enable_if<std::__and_<std::is_convertible<std::basic_string_view<char, std::char_traits<char> > const&, std::basic_string_view<char, std::char_traits<char> > >, std::__not_<std::is_convertible<std::basic_string_view<char, std::char_traits<char> > const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*> >, std::__not_<std::is_convertible<std::basic_string_view<char, std::char_traits<char> > const&, char const*> > >::value, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>::type std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::assign<std::basic_string_view<char, std::char_traits<char> > >(std::basic_string_view<char, std::char_traits<char> > const&) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xf05ff)
    #6 0x558b2d49072b in std::enable_if<std::__and_<std::is_convertible<std::basic_string_view<char, std::char_traits<char> > const&, std::basic_string_view<char, std::char_traits<char> > >, std::__not_<std::is_convertible<std::basic_string_view<char, std::char_traits<char> > const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*> >, std::__not_<std::is_convertible<std::basic_string_view<char, std::char_traits<char> > const&, char const*> > >::value, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&>::type std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::operator=<std::basic_string_view<char, std::char_traits<char> > >(std::basic_string_view<char, std::char_traits<char> > const&) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xe472b)
    #7 0x558b2d4d5c53 in cycfi::elements::basic_label_base<cycfi::elements::default_label>::set_text(std::basic_string_view<char, std::char_traits<char> >) /home/xeverous/workspace/elements/lib/include/elements/element/label.hpp:48
    #8 0x558b2d4d6a39 in operator() /home/xeverous/workspace/elements/lib/src/element/gallery/menu.cpp:95
    #9 0x558b2d4d7995 in _M_invoke /usr/include/c++/8/bits/std_function.h:297
    #10 0x558b2d4ecdbd in std::function<void ()>::operator()() const /usr/include/c++/8/bits/std_function.h:687
    #11 0x558b2d4eb46a in cycfi::elements::basic_menu_item_element::click(cycfi::elements::context const&, cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/src/element/menu.cpp:238
    #12 0x558b2d4c2abd in cycfi::elements::composite_base::click(cycfi::elements::context const&, cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/src/element/composite.cpp:85
    #13 0x558b2d4c2abd in cycfi::elements::composite_base::click(cycfi::elements::context const&, cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/src/element/composite.cpp:85
    #14 0x558b2d4f483d in cycfi::elements::proxy_base::click(cycfi::elements::context const&, cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/src/element/proxy.cpp:82
    #15 0x558b2d4edcc4 in cycfi::elements::basic_popup_element::click(cycfi::elements::context const&, cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/src/element/popup.cpp:29
    #16 0x558b2d4c2abd in cycfi::elements::composite_base::click(cycfi::elements::context const&, cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/src/element/composite.cpp:85
    #17 0x558b2d50fdd1 in cycfi::elements::indirect<cycfi::elements::reference<cycfi::elements::composite<std::vector<std::shared_ptr<cycfi::elements::element>, std::allocator<std::shared_ptr<cycfi::elements::element> > >, cycfi::elements::layer_element> > >::click(cycfi::elements::context const&, cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/include/elements/element/indirect.hpp:190
    #18 0x558b2d4f483d in cycfi::elements::proxy_base::click(cycfi::elements::context const&, cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/src/element/proxy.cpp:82
    #19 0x558b2d5062c5 in operator()<cycfi::elements::context, cycfi::elements::scale_element<cycfi::elements::indirect<cycfi::elements::reference<cycfi::elements::composite<std::vector<std::shared_ptr<cycfi::elements::element> >, cycfi::elements::layer_element> > > > > /home/xeverous/workspace/elements/lib/src/view.cpp:217
    #20 0x558b2d506415 in call<cycfi::elements::view::click(cycfi::elements::mouse_button)::<lambda(const auto:11&, auto:12&)>, cycfi::elements::view> /home/xeverous/workspace/elements/lib/src/view.cpp:107
    #21 0x558b2d50546a in cycfi::elements::view::click(cycfi::elements::mouse_button) /home/xeverous/workspace/elements/lib/src/view.cpp:214
    #22 0x558b2d519ddd in on_button /home/xeverous/workspace/elements/lib/host/linux/base_view.cpp:167
    #23 0x7fe0617237fa  (/usr/lib/x86_64-linux-gnu/libgtk-3.so.0+0x2317fa)
    #24 0x7fe0608fc10c in g_closure_invoke (/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0+0x1010c)
    #25 0x7fe06090f05d  (/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0+0x2305d)
    #26 0x7fe0609170ae in g_signal_emit_valist (/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0+0x2b0ae)
    #27 0x7fe06091812e in g_signal_emit (/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0+0x2c12e)
    #28 0x7fe06186b533  (/usr/lib/x86_64-linux-gnu/libgtk-3.so.0+0x379533)
    #29 0x7fe06172086d  (/usr/lib/x86_64-linux-gnu/libgtk-3.so.0+0x22e86d)
    #30 0x7fe061722947 in gtk_main_do_event (/usr/lib/x86_64-linux-gnu/libgtk-3.so.0+0x230947)
    #31 0x7fe061233764  (/usr/lib/x86_64-linux-gnu/libgdk-3.so.0+0x37764)
    #32 0x7fe061263f91  (/usr/lib/x86_64-linux-gnu/libgdk-3.so.0+0x67f91)
    #33 0x7fe060621416 in g_main_context_dispatch (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c416)
    #34 0x7fe06062164f  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c64f)
    #35 0x7fe0606216db in g_main_context_iteration (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c6db)
    #36 0x7fe060be2efc in g_application_run (/usr/lib/x86_64-linux-gnu/libgio-2.0.so.0+0xa2efc)
    #37 0x558b2d51062e in cycfi::elements::app::run() /home/xeverous/workspace/elements/lib/host/linux/app.cpp:101
    #38 0x558b2d46aeb6 in main /home/xeverous/workspace/elements/examples/empty/main.cpp:70
    #39 0x7fe05ed24b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #40 0x558b2d467b29 in _start (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xbbb29)

0x606000001850 is located 48 bytes inside of 64-byte region [0x606000001820,0x606000001860)
freed by thread T0 here:
    #0 0x7fe0623e3a50 in operator delete(void*) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xf0a50)
    #1 0x558b2d4aacbf in __gnu_cxx::new_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::deallocate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, unsigned long) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xfecbf)
    #2 0x558b2d4a8b3c in std::allocator_traits<std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::deallocate(std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, unsigned long) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xfcb3c)
    #3 0x558b2d4a5505 in std::_Vector_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_M_deallocate(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, unsigned long) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xf9505)
    #4 0x558b2d49d4e8 in std::_Vector_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::~_Vector_base() (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xf14e8)
    #5 0x558b2d491ce7 in std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::~vector() (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xe5ce7)
    #6 0x558b2d46978f in make_elements(cycfi::elements::view&, user_state&) /home/xeverous/workspace/elements/examples/empty/main.cpp:43
    #7 0x558b2d46ad59 in main /home/xeverous/workspace/elements/examples/empty/main.cpp:61
    #8 0x7fe05ed24b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

previously allocated by thread T0 here:
    #0 0x7fe0623e2ba0 in operator new(unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xefba0)
    #1 0x558b2d4aad1e in __gnu_cxx::new_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::allocate(unsigned long, void const*) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xfed1e)
    #2 0x558b2d4a8b7f in std::allocator_traits<std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::allocate(std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >&, unsigned long) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xfcb7f)
    #3 0x558b2d4a558d in std::_Vector_base<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_M_allocate(unsigned long) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xf958d)
    #4 0x558b2d49d787 in void std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::_M_range_initialize<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, std::forward_iterator_tag) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xf1787)
    #5 0x558b2d491a52 in std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >::vector(std::initializer_list<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > const&) (/home/xeverous/workspace/elements/build/examples/empty/EmptyStarter+0xe5a52)
    #6 0x558b2d469417 in make_elements(cycfi::elements::view&, user_state&) /home/xeverous/workspace/elements/examples/empty/main.cpp:43
    #7 0x558b2d46ad59 in main /home/xeverous/workspace/elements/examples/empty/main.cpp:61
    #8 0x7fe05ed24b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

SUMMARY: AddressSanitizer: heap-use-after-free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0x3f866)

My suspection - the selector is capturing string view only but the provided strings are freed when make_menu function returns

elements/lib/src/element/gallery/menu.cpp

Line 93 in a0dd1e6

e.on_click = [btn_text = r.second, on_select, text = items[i]]()

elements/lib/include/elements/element/gallery/menu.hpp

Lines 147 to 151 in a0dd1e6

std::string_view
operator[](std::size_t index) const override
{
return _seq[index];
}

@djowel
Copy link
Member

djowel commented Jun 12, 2020

This is not an MCVE is it? It seems the button (make_button and related code) can be removed?

@djowel
Copy link
Member

djowel commented Jun 12, 2020

I think the issue here is the lifetime of the menu item strings. You are responsible in making them 'alive' in the duration of the menu.

@djowel
Copy link
Member

djowel commented Jun 12, 2020

I think the issue here is the lifetime of the menu item strings. You are responsible in making them 'alive' in the duration of the menu.

Yes, confirmed. You need to hold the vector of strings in the entire duration of the menu. The selection menu does not hold the strings internally.

@djowel
Copy link
Member

djowel commented Jun 12, 2020

This is the corrected code:

#include <elements.hpp>

using namespace cycfi::elements;

// Main window background color
auto constexpr bkd_color = rgba(35, 35, 37, 255);
auto background = box(bkd_color);

struct user_state
{
   std::string menu_item;
};

template <typename Subject>
proxy<cycfi::remove_cvref_t<Subject>>
make_proxy(Subject&& subject)
{
   return { std::forward<Subject>(subject) };
}

auto make_menu(const std::vector<std::string>& names, user_state& state)
{
   return selection_menu(
            [&state](std::string_view selected)
            { state.menu_item = selected; },
            names).first;
}

auto make_elements(view& view, user_state& state, std::vector<std::string> const& names)
{
   auto m = share(make_proxy(make_menu(names, state)));
   return m;
}

int main(int argc, char* argv[])
{
   app _app(argc, argv);
   window _win(_app.name());
   _win.on_close = [&_app]() { _app.stop(); };

   user_state state;
   std::vector<std::string> names = { "One", "Two" };

   view view_(_win);

   view_.content(
      make_elements(view_, state, names),
      background
   );

   _app.run();
   return 0;
}

@djowel djowel closed this as completed Jun 12, 2020
@Xeverous
Copy link
Contributor Author

This is not an MCVE is it? It seems the button (make_button and related code) can be removed?

Yes, I just wanted to make something compileable fast, did not realize I could strip more code.

I think the issue here is the lifetime of the menu item strings. You are responsible in making them 'alive' in the duration of the menu.

That's ... unintuitive. This is the first element I encounter that does not keep the lifetime of used strings.

Anyway, I'm currently doing other things but I'm gathering information for an issue about menus - I think there is a lot to improve on their API and factories.

@djowel
Copy link
Member

djowel commented Jun 12, 2020 •
edited

That's ... unintuitive. This is the first element I encounter that does not keep the lifetime of used strings.

This is not a basic element. This is a gallery composition. Moreover, we want to keep element compositions as light as possible. IMO, this is simply a matter of documentation.

@djowel
Copy link
Member

djowel commented Jun 12, 2020

That's ... unintuitive. This is the first element I encounter that does not keep the lifetime of used strings.

This is not a basic element. This is a gallery composition. Moreover, we want to keep element compositions as light as possible. IMO, this is simply a matter of documentation.

Actually, I take that back... You might be right. The menu items should be holding the strings already in the labels. I'm re-opening this for further investigation.

@djowel djowel reopened this Jun 12, 2020
@djowel
Copy link
Member

djowel commented Jun 12, 2020

That's ... unintuitive. This is the first element I encounter that does not keep the lifetime of used strings.

This is not a basic element. This is a gallery composition. Moreover, we want to keep element compositions as light as possible. IMO, this is simply a matter of documentation.

Actually, I take that back... You might be right. The menu items should be holding the strings already in the labels. I'm re-opening this for further investigation.

OK, I have a fix :-) I'll push later... Thanks you for your attention to detail!

@Xeverous
Copy link
Contributor Author

Another small thing: if the supplied sequence is empty, some actions on the created menu crash (no .menu installed, null pointer dereference)

elements/lib/src/element/gallery/menu.cpp

Lines 87 to 103 in a0dd1e6

if (items.size())
{
vtile_composite list;
for (std::size_t i = 0; i != items.size(); ++i)
{
auto e = menu_item(std::string(items[i]));
e.on_click = [btn_text = r.second, on_select, text = items[i]]()
{
btn_text->set_text(text);
on_select(text);
};
list.push_back(share(e));
}
auto menu = layer(list, panel{});
r.first.menu(menu);
}

And btw, I think if (!items.empty()) is much better (no implicit convertion and clearly expresses the intent, I always check surrounding code if size was supposed to be compared or done something more with).

@djowel
Copy link
Member

djowel commented Jun 12, 2020 •
edited

And btw, I think if (!items.empty()) is much better (no implicit convertion and clearly expresses the intent, I always check surrounding code if size was supposed to be compared or done something more with).

items (menu_selector) does not have empty(). it is not a standard container. it's trivial to add one, but I don't think it is necessary. menu_selector is just an abstraction.

djowel added a commit that referenced this issue Jun 12, 2020
@djowel
Fix for #147
f8b7492
djowel added a commit that referenced this issue Jun 12, 2020
@djowel
Check if _popup is null! See #147
3523195
@djowel
Copy link
Member

djowel commented Jun 12, 2020

Fixed in develop.

Another small thing: if the supplied sequence is empty, some actions on the created menu crash (no .menu installed, null pointer dereference)

Fixed this as well.

@djowel
Copy link
Member

djowel commented Jun 12, 2020

Please close after you verify the fix. Also, please check the "Menus" example. I think I am getting an ASAN false positive similar to this: google/sanitizers#362. I need to investigate my ASAN setup.

@Xeverous
Copy link
Contributor Author

Both my MCVE and the menu example work, on Windows and Linux. No ASAN problems.

@djowel
Copy link
Member

djowel commented Jun 12, 2020

Both my MCVE and the menu example work, on Windows and Linux. No ASAN problems.

Thank you, @Xeverous. And thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@djowel @Xeverous