Fix #92621: foreach_get/set doesn't work for enum #115508

Thomas Barlow · 2023-11-28T07:19:25+01:00

Thomas Barlow commented

2023-11-28 07:19:25 +01:00

There were two issues at play here.

The first is that only enum properties with raw array access were
supported.

The second is that if the item property pointer was found immediately,
rna_raw_access could fall back to the slower loop without checking if
the property was an enum property, which the slower loop did not
support. This would cause the code to go through the entire slower loop
without doing anything. In the case of foreach_get without a
compatible buffer, this meant that the temporary, uninitialized array
(which was supposed to be set by rna_raw_access) would be converted to
Python integers, causing garbage values to be retrieved by
foreach_get, since they were created from uninitialized memory.

This patch adds support for enum properties that are not arrays and adds
asserts if an enum array property is encountered because they do not
exist.

Additional unreachable asserts have been added to the default case of
each switch block within the slower loop because there are prior checks
that the property type is supported, so the switch blocks should never
get an unsupported property type.

~~I'm not sure if enum array properties are even supported in general, in which case, it might be a good idea to add an assertion.~~ edit: Asserts have been added and the PR description has been updated.

There were two issues at play here. The first is that only enum properties with raw array access were supported. The second is that if the item property pointer was found immediately, `rna_raw_access` could fall back to the slower loop without checking if the property was an enum property, which the slower loop did not support. This would cause the code to go through the entire slower loop without doing anything. In the case of `foreach_get` without a compatible buffer, this meant that the temporary, uninitialized array (which was supposed to be set by `rna_raw_access`) would be converted to Python integers, causing garbage values to be retrieved by `foreach_get`, since they were created from uninitialized memory. This patch adds support for enum properties that are not arrays and adds asserts if an enum array property is encountered because they do not exist. Additional unreachable asserts have been added to the default case of each switch block within the slower loop because there are prior checks that the property type is supported, so the switch blocks should never get an unsupported property type. --- ~~I'm not sure if enum array properties are even supported in general, in which case, it might be a good idea to add an assertion.~~ edit: Asserts have been added and the PR description has been updated.

Thomas Barlow added 1 commit 2023-11-28 07:19:42 +01:00

Fix #92621 : foreach_get/set doesn't work for enum

buildbot/vexp-code-patch-coordinator Build done.

Details

c80bfb8c38

Bastien Montagne commented

2023-12-04 15:25:27 +01:00

@blender-bot build

Bastien Montagne approved these changes 2023-12-04 15:30:02 +01:00

Bastien Montagne left a comment

This LGTM, though would not mind having a second eye on it as well, @brecht or @ideasman42 ?

Bastien Montagne requested review from Brecht Van Lommel 2023-12-04 15:30:15 +01:00

Bastien Montagne requested review from Campbell Barton 2023-12-04 15:30:16 +01:00

Brecht Van Lommel requested changes 2023-12-04 15:39:26 +01:00

Brecht Van Lommel left a comment

Enum array properties do not exist, they can not be created with either the C or Python API.

That should be an assert rather than an error message.

Enum array properties do not exist, they can not be created with either the C or Python API. That should be an assert rather than an error message.

👍 1

Thomas Barlow added 2 commits 2023-12-05 00:53:45 +01:00

Merge remote-tracking branch 'origin/main' into fix_non_raw_enum_foreach_getset c4a1a57c52

Replace enum array property error messages with asserts 687fbe1ef8

Thomas Barlow commented

2023-12-05 01:05:56 +01:00

I've replaced the enum array error messages with asserts.

I think I could also add BLI_assert_unreachable(); to the default case of each of the switch blocks?

I've replaced the enum array error messages with asserts. I think I could also add `BLI_assert_unreachable();` to the default case of each of the switch blocks?

Brecht Van Lommel approved these changes 2023-12-08 21:17:23 +01:00

Brecht Van Lommel left a comment

Adding BLI_assert_unreachable() as well sounds good.

Adding `BLI_assert_unreachable()` as well sounds good.

Thomas Barlow added 1 commit 2023-12-09 01:41:34 +01:00

Merge branch 'main' into fix_non_raw_enum_foreach_getset 99fb16e1b1

Thomas Barlow added 1 commit 2023-12-09 01:42:04 +01:00

Add BLI_assert_unreachable() to each switch block's default case

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

fc19c360ed

These should be unreachable because the property's type will already
have been checked for being a supported type and enum array properties
should not exist.

~~Thomas Barlow referenced this pull request 2023-12-09 02:13:25 +01:00~~

Fix #109024: Off-by-1 in rna_access for non-array props without raw access #115967

Brecht Van Lommel approved these changes 2023-12-11 11:24:35 +01:00

Bastien Montagne approved these changes 2023-12-11 12:02:25 +01:00

Bastien Montagne commented

2023-12-11 12:03:21 +01:00

@blender-bot build

Brecht Van Lommel merged commit e503536775 into main

2023-12-12 15:43:33 +01:00

Brecht Van Lommel deleted branch fix_non_raw_enum_foreach_getset

2023-12-12 15:43:35 +01:00

Brecht Van Lommel referenced this issue from a commit