Heap buffer overflow when removing keymap entries on add-on unregister #86431

New Issue

Sybren A. Stüvel · 2021-03-09T16:20:07+01:00

Sybren A. Stüvel commented

2021-03-09 16:20:07 +01:00

System Information
Operating system: Linux-5.4.0-66-generic-x86_64-with-glibc2.31 64 Bits
Graphics card: GeForce RTX 2080/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 450.102.04

Blender Version
Broken: version: 2.93.0 Alpha, branch: master, commit date: 2021-03-09 12:55, hash: 7f9b6b1dc9
Worked: the commit before 9e09214979

Short description of error
When closing Blender, ASAN complains about a heap-buffer-overflow. This happens in the addon_utils.disable_all() call from WM_exit_ex().

9e09214979 adds a check that pointers to temporary datablocks are not assigned to anything: value_owner_id->tag & LIB_TAG_TEMP_MAIN. Unfortunately, in the above case (Blender shutting down, addons being unregistered), value_owner_id is no longer valid.

This issue was introduced in 9e09214979. However, I suspect that the underlying issue was already present before, and just comes to light now because of that commit.

Exact steps for others to reproduce the error

Take a debug build of Blender with ASAN enabled
Start Blender
Enable the Bone Selection Sets add-on
Quit Blender

This patch makes it more visible what is exactly being accessed:

diff --git a/release/scripts/modules/addon_utils.py b/release/scripts/modules/addon_utils.py
index 83bed69d8d2..be3a65c6e67 100644
--- a/release/scripts/modules/addon_utils.py
+++ b/release/scripts/modules/addon_utils.py
@@ -492,11 +492,14 @@ def disable_all():
         item for item in sys.modules.items()
         if getattr(item[1], "__addon_enabled__", False)
     ]
+    print("disable_all():")
     - Check the enabled state again since it's possible the disable call
     - of one add-on disables others.
     for mod_name, mod in addon_modules:
         if getattr(mod, "__addon_enabled__", False):
+            print(f"  - {mod_name}", flush=True)
             disable(mod_name)
+    print("Disabled everything", flush=True)
 
 
 def _blender_manual_url_prefix():
diff --git a/source/blender/python/intern/bpy_rna.c b/source/blender/python/intern/bpy_rna.c
index 7a43c9cb997..38e1168c0d8 100644
--- a/source/blender/python/intern/bpy_rna.c
+++ b/source/blender/python/intern/bpy_rna.c
@@ -6238,6 +6238,10 @@ static PyObject *pyrna_func_call(BPy_FunctionRNA *self, PyObject *args, PyObject
       }
     }
 #endif
+    printf("%.200s.%.200s(): accessing \"%.200s\"\n",
+           RNA_struct_identifier(self_ptr->type),
+           RNA_function_identifier(self_func),
+           RNA_property_identifier(parm));
     err = pyrna_py_to_prop(&funcptr, parm, iter.data, item, "");
 
     if (err != 0) {

On my machine it outputs:

disable_all():
  - bone_selection_sets
KeyMapItems.remove(): accessing "item"
  - pose_library_mockup
Disabled everything

**System Information** Operating system: Linux-5.4.0-66-generic-x86_64-with-glibc2.31 64 Bits Graphics card: GeForce RTX 2080/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 450.102.04 **Blender Version** Broken: version: 2.93.0 Alpha, branch: master, commit date: 2021-03-09 12:55, hash: `7f9b6b1dc9` Worked: the commit before 9e09214979 **Short description of error** When closing Blender, ASAN complains about a heap-buffer-overflow. This happens in the `addon_utils.disable_all()` call from `WM_exit_ex()`. 9e09214979 adds a check that pointers to temporary datablocks are not assigned to anything: `value_owner_id->tag & LIB_TAG_TEMP_MAIN`. Unfortunately, in the above case (Blender shutting down, addons being unregistered), `value_owner_id` is no longer valid. This issue was introduced in 9e09214979. However, I suspect that the underlying issue was already present before, and just comes to light now because of that commit. **Exact steps for others to reproduce the error** * Take a debug build of Blender with ASAN enabled * Start Blender * Enable the Bone Selection Sets add-on * Quit Blender This patch makes it more visible what is exactly being accessed: ``` diff --git a/release/scripts/modules/addon_utils.py b/release/scripts/modules/addon_utils.py index 83bed69d8d2..be3a65c6e67 100644 --- a/release/scripts/modules/addon_utils.py +++ b/release/scripts/modules/addon_utils.py @@ -492,11 +492,14 @@ def disable_all(): item for item in sys.modules.items() if getattr(item[1], "__addon_enabled__", False) ] + print("disable_all():") - Check the enabled state again since it's possible the disable call - of one add-on disables others. for mod_name, mod in addon_modules: if getattr(mod, "__addon_enabled__", False): + print(f" - {mod_name}", flush=True) disable(mod_name) + print("Disabled everything", flush=True) def _blender_manual_url_prefix(): diff --git a/source/blender/python/intern/bpy_rna.c b/source/blender/python/intern/bpy_rna.c index 7a43c9cb997..38e1168c0d8 100644 --- a/source/blender/python/intern/bpy_rna.c +++ b/source/blender/python/intern/bpy_rna.c @@ -6238,6 +6238,10 @@ static PyObject *pyrna_func_call(BPy_FunctionRNA *self, PyObject *args, PyObject } } #endif + printf("%.200s.%.200s(): accessing \"%.200s\"\n", + RNA_struct_identifier(self_ptr->type), + RNA_function_identifier(self_func), + RNA_property_identifier(parm)); err = pyrna_py_to_prop(&funcptr, parm, iter.data, item, ""); if (err != 0) { ``` On my machine it outputs: ``` disable_all(): - bone_selection_sets KeyMapItems.remove(): accessing "item" - pose_library_mockup Disabled everything ```

Sybren A. Stüvel commented

2021-03-09 16:20:07 +01:00

Added subscriber: @dr.sybren

Sybren A. Stüvel commented

2021-03-09 16:20:30 +01:00

Added subscriber: @ideasman42

Robert Guetzkow commented

2021-03-09 17:08:46 +01:00

Added subscriber: @rjg

Campbell Barton commented

2021-03-10 13:43:44 +01:00

Notes PointerRNA.owner_id of window manager become invalid when loading a new file.

Firstly, the invalid ID pointer has been happening for years, surprising the error only shows up now.

It seems the API functions for keymaps don't use this and access G.main.wm.first instead, so we didn't run into this problem before.

Unfortunately as far as I can see all solutions have some significant trade-offs.

Remove this check (temporary ID's can then be assigned when this shouldn't be allowed).

The invalid ID problem will likely bite us later on, it's OK short term.
Keep the window-manager at the same memory location on file load P2027

This means any pointers to the window-manager in the file will become invalid, although by default AFAICS, there are no pointers that do this, besides ID-properties which may reference the window-manager ID.
Perform a hack that reassigns window-manager ID pointers on file load, using gc.get_objects() for example, could be slow - mentioning for completeness. I'd need to check this would work reliably too, it does detect the keymap items from this report.
Change the API usage not to store the keymap items, instead use strings for example - that can be used to lookup the keymap items for removal

(Also not nice as this is the documented way to add/remove keymap items, mentioning for completeness).

Notes `PointerRNA.owner_id` of window manager become invalid when loading a new file. Firstly, the invalid ID pointer has been happening for years, surprising the error only shows up now. It seems the API functions for keymaps don't use this and access `G.main.wm.first` instead, so we didn't run into this problem before. Unfortunately as far as I can see all solutions have some significant trade-offs. - Remove this check (temporary ID's can then be assigned when this shouldn't be allowed). *The invalid ID problem will likely bite us later on, it's OK short term.* - Keep the window-manager at the same memory location on file load [P2027](https://archive.blender.org/developer/P2027.txt) *This means any pointers to the window-manager in the file will become invalid, although by default AFAICS, there are no pointers that do this, besides ID-properties which may reference the window-manager ID.* - Perform a hack that reassigns window-manager ID pointers on file load, using `gc.get_objects()` for example, could be slow - mentioning for completeness. I'd need to check this would work reliably too, it does detect the keymap items from this report. - Change the API usage not to store the keymap items, instead use strings for example - that can be used to lookup the keymap items for removal *(Also not nice as this is the documented way to add/remove keymap items, mentioning for completeness).*

Robert Guetzkow commented

2021-03-10 14:40:04 +01:00

Change the API usage not to store the keymap items, instead use strings for example - that can be used to lookup the keymap items for removal

This idea may actually be preferable because it could also prevent bugs like #83099 from happening.

> Change the API usage not to store the keymap items, instead use strings for example - that can be used to lookup the keymap items for removal This idea may actually be preferable because it could also prevent bugs like [#83099 ](https://developer.blender.org/T83099) from happening.

Bastien Montagne commented

2021-03-10 14:46:15 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2021-03-10 14:46:15 +01:00

Think the swap option is the best here, though it needs to be refined compared to the proposed P2027:

You do need to remap, not only custom props can use a WindowManager, but also drivers...
Assert that there is no Main.relations in either Main's..

Think the swap option is the best here, though it needs to be refined compared to the proposed [P2027](https://archive.blender.org/developer/P2027.txt): * You do need to remap, not only custom props can use a WindowManager, but also drivers... * Assert that there is no `Main.relations` in either Main's..

Campbell Barton commented

2021-03-11 04:53:19 +01:00

@mont29 submitted D10690: Fix #86431: Keep the memory location of the window manager when loading a file which does the remap, although I'm a skeptical about how useful it is to reference windowing-data from ID properties in general.

@mont29 submitted [D10690: Fix #86431: Keep the memory location of the window manager when loading a file](https://archive.blender.org/developer/D10690) which does the remap, although I'm a skeptical about how useful it is to reference windowing-data from ID properties in general.

blender-admin commented

2021-03-11 14:34:23 +01:00

This issue was referenced by 2cc5af9c55

This issue was referenced by 2cc5af9c553cfc00b7d4616445ad954597a92d94

Campbell Barton commented

2021-03-11 14:51:46 +01:00

Changed status from 'Needs Triage' to: 'Resolved'

Campbell Barton closed this issue

2021-03-11 14:51:46 +01:00

Campbell Barton self-assigned this 2021-03-11 14:51:46 +01:00

Thomas Dinges added this to the 2.93 LTS milestone 2023-02-07 18:46:16 +01:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Heap buffer overflow when removing keymap entries on add-on unregister #86431