Page MenuHome

Versioning: Try to prevent accidental early-exiting in versioning
Needs ReviewPublic

Authored by Julian Eisel (Severin) on Wed, Sep 16, 4:40 PM.

Details

Reviewers
None
Group Reviewers
Data, Assets & I/O
Summary

In a recent commit, a return was added in the middle of regular versioning
code, causing unintended early-exit of versioning (fixed in rB1a4fc6dcd67b).
This can be a rather hard to track down error, which would be good to avoid -
statically if possible.

Poisoning occurrences of "return" is the only static way I can think of to do
this. Two trade-offs: It's GCC and Clang only, and all functions to be affected
should be at the end of a file (there's no way to "end" the scope of poisoning).
Maybe this is a bit overkill because the error is unlikely to happen often, but
I'd like to have it proposed nevertheless. After all this may prevent violated
invariants ending up in files.

Diff Detail

Repository
rB Blender
Branch
temp-versioning-poison-early-exit (branched from master)
Build Status
Buildable 10238
Build 10238: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) requested review of this revision.Wed, Sep 16, 4:40 PM
source/blender/blenloader/intern/versioning.h
26

AFAIK it's poisoning is actually supported by GCC & Clang. Should be corrected if so.

34

BLO_versioning_multipatch_begin is a rather awkward name, suggestions welcome.

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Wed, Sep 16, 4:47 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)Wed, Sep 16, 4:54 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)

Not strongly opposed to that, but not really a fan of it either, I find it rather cumbersome and adding annoying constraints, for a rather limited benefit.

Note that the usage of return in the original buggy commit was actually a bug for its own logic as well, since even if it had been in its own local function, it would have prevented proper action to take place (skipping all further fcurves and actions)!

That kind of logic mistake always happen, we try to reduce them with review, but… Poisoning return like that would have revealed it in this specific case, but would not have been detected it in any other place.

Why not use define for this?

#define return BLI_STATIC_ASSERT(false, "No return allowed")

While it would be nice if it could be defined in a header, at least it's straightforward and works everywhere.


This is already used in Blender to prevent access to U in user preferences versioning code.

That kind of logic mistake always happen, we try to reduce them with review, but… Poisoning return like that would have revealed it in this specific case, but would not have been detected it in any other place.

But the issue here is that if the error is present in one version patch, it also affects the following version patches - which may or may not be executed then. That could of course break things quite badly and all you have to do for that is open an old file. So damage mitigation is a good idea IMHO.
I'd actually consider this a design issue with the current versioning system. There should not be a way for one version patch to accidentally prevent execution of another one.
I may work on that during the next code quality day: Each version patch could just be its own callback, registered for a specific version number and executed independently.

Why not use define for this?

#define return BLI_STATIC_ASSERT(false, "No return allowed")

While it would be nice if it could be defined in a header, at least it's straightforward and works everywhere.


This is already used in Blender to prevent access to U in user preferences versioning code.

Actually we use #define U (_error) there, which we could use here too. I didn't like this at first but a nice benefit is that we can #undef it. So each versioning function could be protected with this:

diff --git a/source/blender/blenloader/intern/versioning_290.c b/source/blender/blenloader/intern/versioning_290.c
index 8a5f77fac1d..f146c61d7f3 100644
--- a/source/blender/blenloader/intern/versioning_290.c
+++ b/source/blender/blenloader/intern/versioning_290.c
@@ -328,6 +328,9 @@ void blo_do_versions_290(FileData *fd, Library *UNUSED(lib), Main *bmain)
 {
   UNUSED_VARS(fd);
 
+/* Prevent accidental use of "return" within the versioning. Using it may cause bad trouble. */
+#define return (_error_)
+
   /** Repair files from duplicate brushes added to blend files, see: T76738. */
   if (!MAIN_VERSION_ATLEAST(bmain, 290, 2)) {
     {
@@ -691,4 +694,7 @@ void blo_do_versions_290(FileData *fd, Library *UNUSED(lib), Main *bmain)
   {
     /* Keep this block, even when empty. */
   }
+
+  /* "return" may be used now again. Keep at the very end! */
+#undef return
 }