Fix rare crashes when reading files with multi-levels of libraries. #114784

Bastien Montagne · 2023-11-13T12:13:47+01:00

Bastien Montagne commented

2023-11-13 12:13:47 +01:00

Some left-over code from old refactoring/improvements of old addresses
remapping in library reading code was causing some wrong remapping
between different IDs.

The code was actually not doing what its comment was describing, since
it would cause a random remapping to a different new address than the
expected one:

Initial state, before removed buggy line:
old_addr_1 -> new_addr_1
old_addr_2 -> new_addr_2

End state, if old_addr_2 == new_addr_1:
old_addr_1 -> new_addr_2
old_addr_2 -> new_addr_2

From the description of the removed line, that behavior was actually
already covered by adding the remapping rule in the line above, since
adding a remapping overwrite an existing one if needed, so the line
above would do (in case a placeholder ID existed before the actual ID
got read):

Initial state, before adding new remapping rule:
old_addr_1 -> new_addr_placeholder_1

End state, after adding remapping to newly read ID:
old_addr_1 -> new_addr_1

NOTE: Noticed thanks to rare failures of the liboverride tests on
buildbots. The issue was less than 1%-reproducible with a debug build
and ASAN, but had about 2-4% repducibility with release builds.

Some left-over code from old refactoring/improvements of old addresses remapping in library reading code was causing some wrong remapping between different IDs. The code was actually not doing what its comment was describing, since it would cause a random remapping to a different new address than the expected one: Initial state, before removed buggy line: `old_addr_1 -> new_addr_1` `old_addr_2 -> new_addr_2` End state, if `old_addr_2` == `new_addr_1`: `old_addr_1 -> new_addr_2` `old_addr_2 -> new_addr_2` From the description of the removed line, that behavior was actually already covered by adding the remapping rule in the line above, since adding a remapping overwrite an existing one if needed, so the line above would do (in case a placeholder ID existed before the actual ID got read): Initial state, before adding new remapping rule: `old_addr_1 -> new_addr_placeholder_1` End state, after adding remapping to newly read ID: `old_addr_1 -> new_addr_1` NOTE: Noticed thanks to rare failures of the liboverride tests on buildbots. The issue was less than 1%-reproducible with a debug build and ASAN, but had about 2-4% repducibility with release builds.

Bastien Montagne added 1 commit 2023-11-13 12:13:58 +01:00

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

1caa176f32 Fix rare crashes when reading files with multi-levels of libraries.

Some left-over code from old refactoring/improvements of old addresses
remapping in library reading code was causing some wrong remapping
between different IDs.

The code was actually not doing what its comment was describing, since
it would cause a random remapping to a different new address than the
expected one:

Initial state, before removed buggy line:
`old_addr_1 -> new_addr_1`
`old_addr_2 -> new_addr_2`

End state, if `old_addr_2` == `new_addr_1`:
`old_addr_1 -> new_addr_2`
`old_addr_2 -> new_addr_2`

From the description of the removed line, that behavior was actually
already covered by adding the remapping rule in the line above, since
adding a remapping overwrite an existing one if needed, so the line
above would do (in case a placeholder ID existed before the actual ID
got read):

Initial state, before adding new remapping rule:
`old_addr_1 -> new_addr_placeholder_1`

End state, after adding remapping to newly read ID:
`old_addr_1 -> new_addr_1`

NOTE: Noticed thanks to rare failures of the liboverride tests on
buildbots. The issue was less than 1%-reproducible with a debug build
and ASAN, but had about 2-4% repducibility with release builds.

Bastien Montagne commented

2023-11-13 12:14:11 +01:00

@blender-bot build

Bastien Montagne requested review from Brecht Van Lommel 2023-11-13 12:14:22 +01:00

Bastien Montagne commented

2023-11-13 12:17:02 +01:00

NOTE: I would not risk this change into 4.0 at this point, since:

Any change there can potentially have dreadful consequences
Existing issue is likely very rarely happening in real-life production situation. At least it was never reported.

Could be a good candidate for 4.0.1 though.

CC @ThomasDinges FYI.

NOTE: I would not risk this change into 4.0 at this point, since: * Any change there can potentially have dreadful consequences * Existing issue is likely very rarely happening in real-life production situation. At least it was never reported. Could be a good candidate for 4.0.1 though. CC @ThomasDinges FYI.

Brecht Van Lommel approved these changes 2023-11-13 13:01:23 +01:00

Bastien Montagne merged commit d94281d7af into main

2023-11-13 15:02:11 +01:00

Bastien Montagne referenced this issue from a commit

2023-11-13 15:02:11 +01:00

Fix rare crashes when reading files with multi-levels of libraries.

Bastien Montagne deleted branch tmp-fix-readfile-crash

2023-11-13 15:02:13 +01:00

Bastien Montagne referenced this pull request

2023-11-13 15:06:25 +01:00

4.0: Candidates for corrective releases #114706

Thomas Dinges referenced this issue from a commit