Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static fields not validated correctly #418

Open
jamesu opened this issue Jan 29, 2019 · 8 comments
Open

Static fields not validated correctly #418

jamesu opened this issue Jan 29, 2019 · 8 comments

Comments

@jamesu
Copy link
Contributor

jamesu commented Jan 29, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

jamesu commented Jan 29, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

jamesu commented Jan 29, 2019

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
Copy link
Contributor

greenfire27 commented Jan 29, 2019

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
Copy link
Contributor Author

jamesu commented Jan 29, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

jamesu commented Jan 29, 2019

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" ;)

greenfire27 added a commit that referenced this issue Jan 30, 2019
This fixes issue #418 where the validators for a field are not firing in most cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants