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.

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

memory leak HardwareBreakpoints::cpuContextMenu and friends #744

Open
sorokin opened this issue Feb 15, 2020 · 5 comments
Open

memory leak HardwareBreakpoints::cpuContextMenu and friends #744

sorokin opened this issue Feb 15, 2020 · 5 comments

Comments

@sorokin
Copy link
Contributor

@sorokin sorokin commented Feb 15, 2020

Leak sanitizer reports these leaks:

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long) (/nix/store/784rh7jrfhagbkydjfrv68h9x3g4gqmk-gcc-8.3.0-lib/lib/libasan.so.5+0xedd80)
    #1 0x7f5c7e695267 in HardwareBreakpointsPlugin::HardwareBreakpoints::stackContextMenu() /home/ivan/d/edb-debugger/plugins/HardwareBreakpoints/HardwareBreakpoints.cpp:253
    #2 0x694aac in Debugger::Debugger(QWidget*) /home/ivan/d/edb-debugger/src/Debugger.cpp:485
    #3 0x4e683a in start_debugger /home/ivan/d/edb-debugger/src/main.cpp:96
    #4 0x4e683a in main /home/ivan/d/edb-debugger/src/main.cpp:255

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long) (/nix/store/784rh7jrfhagbkydjfrv68h9x3g4gqmk-gcc-8.3.0-lib/lib/libasan.so.5+0xedd80)
    #1 0x7f5c7e69ba38 in HardwareBreakpointsPlugin::HardwareBreakpoints::cpuContextMenu() /home/ivan/d/edb-debugger/plugins/HardwareBreakpoints/HardwareBreakpoints.cpp:324
    #2 0x694aac in Debugger::Debugger(QWidget*) /home/ivan/d/edb-debugger/src/Debugger.cpp:485
    #3 0x4e683a in start_debugger /home/ivan/d/edb-debugger/src/main.cpp:96
    #4 0x4e683a in main /home/ivan/d/edb-debugger/src/main.cpp:255

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long) (/nix/store/784rh7jrfhagbkydjfrv68h9x3g4gqmk-gcc-8.3.0-lib/lib/libasan.so.5+0xedd80)
    #1 0x7f5c7e6985d7 in HardwareBreakpointsPlugin::HardwareBreakpoints::dataContextMenu() /home/ivan/d/edb-debugger/plugins/HardwareBreakpoints/HardwareBreakpoints.cpp:288
    #2 0x694aac in Debugger::Debugger(QWidget*) /home/ivan/d/edb-debugger/src/Debugger.cpp:485
    #3 0x4e683a in start_debugger /home/ivan/d/edb-debugger/src/main.cpp:96
    #4 0x4e683a in main /home/ivan/d/edb-debugger/src/main.cpp:255

The lines in the stack trace corresponds to the lines where QMenu is created:

	auto menu = new QMenu(tr("Hardware Breakpoints"));

Notice that QMenu is created without a parent. At first I thought to fix the problem by passing QWidget* parent to cpuContextMenu and this is fine for usages in addPluginContextMenu. But I stuck at the usage in finishPluginSetup.

What do you think is the best way to fix the problem?

@10110111

This comment has been minimized.

Copy link
Contributor

@10110111 10110111 commented Feb 16, 2020

I suppose for the purpose of cleanliness, the best way is to save the menu as member unique_ptr in class HardwareBreakpoints.

But OTOH, does there exist a scenario when there'll be more than one call to <plugin-name>::dataContextMenu()? If not, we might just as well ignore this "leak", since it doesn't lead to systematic increase in memory consumption over EDB run time.

@sorokin

This comment has been minimized.

Copy link
Contributor Author

@sorokin sorokin commented Feb 16, 2020

If not, we might just as well ignore this "leak", since it doesn't lead to systematic increase in memory consumption over EDB run time.

This approach make it difficult to find real leaks. When you have 1000 false positives and you got one real one it is much more difficult to notice than when you have 0 and you got one.

@sorokin

This comment has been minimized.

Copy link
Contributor Author

@sorokin sorokin commented Feb 16, 2020

I looked at what qactions have shortcuts and apparently there is only one such action: Assemble (Space). I guess I can just introduce a new function something like IPlugin::globalShortcuts to be used in finishPluginSetup. This will limit usages of cpuContextMenu and friends only to the places where real menu need to be created.

@eteran

This comment has been minimized.

Copy link
Owner

@eteran eteran commented Feb 16, 2020

Thanks, I'll check into this

@eteran

This comment has been minimized.

Copy link
Owner

@eteran eteran commented Feb 17, 2020

I think probably the "correct" solution would be for something in Debugger.cpp to take ownership of these QAction objects so they get destroyed when the new owner gets destroyed. I'll think about it for a little bit.

sorokin added a commit to sorokin/edb-debugger that referenced this issue Feb 18, 2020
The issue was about leaks reported by leak sanitizer:

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e695267 in HardwareBreakpointsPlugin::HardwareBreakpoints::stackContextMenu() HardwareBreakpoints.cpp:253
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e69ba38 in HardwareBreakpointsPlugin::HardwareBreakpoints::cpuContextMenu() HardwareBreakpoints.cpp:324
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e6985d7 in HardwareBreakpointsPlugin::HardwareBreakpoints::dataContextMenu() HardwareBreakpoints.cpp:288
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

The most idiomatic way to fix the issue was to assign a parent
to the submenus created in these functions. Unfortunately
there is no suitable parent available in these functions.

This commit fixes the issue in the most straight-forward way
possible: it just adds QMenu* parent parameter to the functions
* IPlugin::cpuContextMenu
* IPlugin::registerContextMenu
* IPlugin::stackContextMenu
* IPlugin::dataContextMenu

This allows plugins to set proper parent to all submenus
they choose to create in order for them to be properly destroyed
when root menu is destroyed.

This commit also sets parents to the QActions that are created
in these functions so they are timedly destroyed. As a possible
follow-up these QActions can be created once in plugin
constructor and not everytime the menu is created.
sorokin added a commit to sorokin/edb-debugger that referenced this issue Feb 18, 2020
The issue was about leaks reported by leak sanitizer:

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e695267 in HardwareBreakpointsPlugin::HardwareBreakpoints::stackContextMenu() HardwareBreakpoints.cpp:253
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e69ba38 in HardwareBreakpointsPlugin::HardwareBreakpoints::cpuContextMenu() HardwareBreakpoints.cpp:324
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e6985d7 in HardwareBreakpointsPlugin::HardwareBreakpoints::dataContextMenu() HardwareBreakpoints.cpp:288
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

The most idiomatic way to fix the issue was to assign a parent
to the submenus created in these functions. Unfortunately
there is no suitable parent available in these functions.

This commit fixes the issue in the most straight-forward way
possible: it just adds QMenu* parent parameter to the functions
* IPlugin::cpuContextMenu
* IPlugin::registerContextMenu
* IPlugin::stackContextMenu
* IPlugin::dataContextMenu

This allows plugins to set proper parent to all submenus
they choose to create in order for them to be properly destroyed
when root menu is destroyed.

This commit also sets parents to the QActions that are created
in these functions so they are timedly destroyed. As a possible
follow-up these QActions can be created once in plugin
constructor and not everytime the menu is created.
sorokin added a commit to sorokin/edb-debugger that referenced this issue Feb 29, 2020
The issue was about leaks reported by leak sanitizer:

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e695267 in HardwareBreakpointsPlugin::HardwareBreakpoints::stackContextMenu() HardwareBreakpoints.cpp:253
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e69ba38 in HardwareBreakpointsPlugin::HardwareBreakpoints::cpuContextMenu() HardwareBreakpoints.cpp:324
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

Indirect leak of 48 byte(s) in 1 object(s) allocated from:
    #0 0x7f5c95377d80 in operator new(unsigned long)
    eteran#1 0x7f5c7e6985d7 in HardwareBreakpointsPlugin::HardwareBreakpoints::dataContextMenu() HardwareBreakpoints.cpp:288
    eteran#2 0x694aac in Debugger::Debugger(QWidget*) Debugger.cpp:485
    eteran#3 0x4e683a in start_debugger main.cpp:96
    eteran#4 0x4e683a in main main.cpp:255

The most idiomatic way to fix the issue was to assign a parent
to the submenus created in these functions. Unfortunately
there is no suitable parent available in these functions.

This commit fixes the issue in the most straight-forward way
possible: it just adds QMenu* parent parameter to the functions
* IPlugin::cpuContextMenu
* IPlugin::registerContextMenu
* IPlugin::stackContextMenu
* IPlugin::dataContextMenu

This allows plugins to set proper parent to all submenus
they choose to create in order for them to be properly destroyed
when root menu is destroyed.

This commit also sets parents to the QActions that are created
in these functions so they are timedly destroyed. As a possible
follow-up these QActions can be created once in plugin
constructor and not everytime the menu is created.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.