Avoid crashes w/ bad weight indices, uint for MDeformWeight's
AbandonedPublic
Actions

Authored by Campbell Barton (campbellbarton) on Jul 27 2018, 1:27 AM.

Details

Reviewers

Bastien Montagne (mont29)
Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)

Maniphest Tasks

T56108: Crash when editing vertex groups in 2.79b and 2.8

Summary

T56108 was reported, the file has bad weight group -2074087088.
We could consider the file corrupt and close the report.

However using an unsigned value here means all range checks, eg: def_nr < group_len, will ignore out of range values.

So this is mostly a policy question for how we should handle invalid data.

An alternative is to check for def_nr >= 0 && def_nr < group_len but think this is too noisy, likely to be missed in new code & easy to forget.

Diff Detail

Repository

rB Blender

Branch

TEMP-T56108-FIX (branched from blender2.8)

Build Status

Buildable 1857
Build 1857: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) created this revision.Jul 27 2018, 1:27 AM

Campbell Barton (campbellbarton) added a task: T56108: Crash when editing vertex groups in 2.79b and 2.8.Jul 27 2018, 1:28 AM

Campbell Barton (campbellbarton) retitled this revision from Use unsigned int's for MDeformWeight's to Avoid crashes w/ bad weight indices, uint for MDeformWeight's.Jul 27 2018, 1:34 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Seems fine.

This revision is now accepted and ready to land.Jul 27 2018, 10:32 AM

Also looks good to me.

By doing this you're opening another pile of nasty things:

Implicit conversion from int to unsigned int, since all things like vgroup_tot are passed as int. This is harmless on functionality vise, but is confusing from readability point of view.
You're loosing sign-niness on assignment to variables, i.e. const int index = dw->def_nr;
You're loosing sign-niness when passing value to defvert_verify_index() and defvert_find_index(). There might be others, didn't check more detailed. Such a casts are even more confusing.
The mesh validate doesn't check for def_nr being in a legit range at all now?

I'm not sure why is it a thing to fix out-of-bounds access by changing data type, but if you do so -- do it globally to avoid confusion of implicit casts all over the place.

But i'm also not sure why there are so many places where group index is to be checked. Would imagine it is to be done on load , if any is to be checked. We don't do this for other indices, so why deformation groups are special here?

This revision now requires changes to proceed.Jul 30 2018, 10:37 AM

@Sergey Sharybin (sergey)

We do checks on weight group indices because the mesh data is only loosely coupled with the object data.

You could for eg, have an object with 10 vgroups, using a linked mesh - which has more when reloaded.
(similar to material index).

When we have an array of vgroups generated from the object, we need to check no indices exceed the maximum.

From checking over the code we don't use negative indices anywhere, but you're right that mixing signed/unsigned isn't great.

We could change const int index = dw->def_nr; for eg to use uint, in most/all places, since these are slots, we're not doing the kinds of arithmetic on them that normally give issues with sign.

On the other hand we can leave this as-is too, since technically this file is corrupt.

@Campbell Barton (campbellbarton), but vgroups are stored in mesh, not on the object?

Another concerning point is that doing such a silent sanity check doesn't tell user that his mesh got corrupt. Guess ideally it should be a warning somewhere. But that's another story.

That being said, if it's uint everywhere for defgroup index think it'll be fine to commit.