Static fields not validated correctly

[jamesu]

Just been backporting some of this code into a tge project. I've noticed In SimObject::setDataField, we have:

         if(array1 >= 0 && array1 < fld->elementCount && fld->elementCount >= 1)
         {
....

            onStaticModified( slotName, value );

            return;
         }
         if(fld->validator)
                  fld->validator->validateType(this, (void *) (((const char *)this) + fld->offset));

The problem is fields will never be validated because in 99% of cases the function will exit at the return. In T3D and other previous versions there is this block before the onStaticModified call:

            if(fld->validator)
               fld->validator->validateType(this, (void *) (((const char *)this) + fld->offset));

I suspect a bad merge may have taken place at one point.

[greenfire27]

If what you're saying is correct, then it seems like we could just remove lines 545 and 547. Maybe that's what was intended.

Can you explain what the side effect of this bug would be?

[jamesu]

The side-effect is if you have a validated field (e.g. addNamedFieldV(lifetime, TypeS32, ProjectileData, new IRangeValidatorScaled(TickMs, 0, Projectile::MaxLivingTicks));) you can set the field to any value and skip the validator.

Removing the two lines should also have the same effect. Probably when someone added the FrameTemp buffer stuff they didn't clean stuff up too well (going back to early tge here) - for ref, the earliest code I can find for this func just has the Con::setData call in the array check, then the validator call, then onStaticModified callback.

[greenfire27]

I see. So if I tried to assign "foo" to your example. Then when the projectile tried to use it's lifetime value as an S32 the engine would crash (or hopefully parse the string into an unknown number and try to keep going).

At any rate, it sounds like we should fix this. I'll check in something for this soon.

[jamesu]

Thx man, just thought I'd let you know since I bumped into it. My particular problem was script was setting 5000 to the lifetime var when it was restricted to 4095. With "foo" you'd probably get 0 as per the standard atoi impl.

[greenfire27]

I was just looking at making this change. Shouldn't the validation happen before the call to con::setData()?

ValidateType()
con::setData()
onStaticModified()

In that order? That way the validator could change the value before it gets set. At least that makes sense to me.

[jamesu]

I believe the original intent of the validator was to change the value after the setData stuff is called. The reason being objects may have custom field handlers which could set the value into an invalid state. For example you could conceivably create a custom handler which accepts, say, strings in a field which usually accepts numbers. Or it could modify the object state in some other way such that the validation will fail after.

Though TBH the whole design of this smells a bit fishy anyway since its possible to have fields which aren't backed by a location in memory which wouldn't normally work with the validator in its current state.

Obv go with what makes more sense and doesn't break anything already existing in the code (I don't think T2D has many things which use the validator though...).

[greenfire27]

Ok, I think we should go with the original design. I have a feeling that validation doesn't mean the same thing to me as it did to the Torque forefathers.

[jamesu]

I think its prob a case of "worked well when it was simple, but we never rethought it when we added all this other trash on top" ;)