Skip to content

[GEN][ZH] Fix undefined behavior in ThingTemplate::isEquivalentTo #932

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

Merged
merged 8 commits into from
Jul 12, 2025

Conversation

Caball009
Copy link

Another case of checking 'this' pointer in a member function like #870 & #878

Bool ThingTemplate::isEquivalentTo(const ThingTemplate* tt) const
{
// sanity
if (!(this && tt))
return false;

This pr fixes the undefined behavior in ThingTemplate::isEquivalentTo by making it a static function.

If the changes are considered acceptable, then it needs to be replicated in Generals. As far as I'm concerned that can be done in this pr as well.

@Mauller
Copy link

Mauller commented May 25, 2025

The better solution here might be to get rid of the equivalent to function and Instead implement an overload for the comparison operator within thingtemplate.

Then do (object->getTemplate() == otherTemplate)

@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels May 25, 2025
@Caball009
Copy link
Author

The better solution here might be to get rid of the equivalent to function and Instead implement an overload for the comparison operator within thingtemplate.

Then do (object->getTemplate() == otherTemplate)

I like that approach.

@xezon
Copy link

xezon commented May 25, 2025

The change also needs to be replicated in Generals

@Caball009 Caball009 force-pushed the fix_this_pointer_thing_template branch 2 times, most recently from 9dad48b to d914978 Compare July 10, 2025 14:48
@Caball009 Caball009 force-pushed the fix_this_pointer_thing_template branch from d914978 to 5c33dbb Compare July 10, 2025 14:51
@Caball009
Copy link
Author

Caball009 commented Jul 10, 2025

Updated and replicated in Generals.

I made the assumption that the ThingTemplate cannot be NULL for Objects and PartitionFilterThings. Afaik, objects are only created in ThingFactory::newObject, which checks if the template is NULL. I checked and all calls to PartitionFilterThings constructor are behind a template != NULL check. I added an assertion to the constructor for good measure.

@Caball009 Caball009 requested a review from xezon July 10, 2025 15:07
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very safe.

@xezon xezon changed the title [ZH] Fix undefined behavior in ThingTemplate::isEquivalentTo [GEN][ZH] Fix undefined behavior in ThingTemplate::isEquivalentTo Jul 12, 2025
@xezon xezon merged commit 808017e into TheSuperHackers:main Jul 12, 2025
15 checks passed
@Caball009 Caball009 deleted the fix_this_pointer_thing_template branch July 12, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants