Skip to content

[GEN][ZH] Remove unnecessary NULL pointer tests in RTS3DScene::RTS3DScene() #1105

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xezon
Copy link

@xezon xezon commented Jun 19, 2025

This change removes unnecessary NULL pointer tests in RTS3DScene::RTS3DScene() to make the compiler happy.

Instead I added tests for the array sizes and only allocate the arrays when the value is larger than 0, which is currently always the case anyway. This does not change game behaviour.

GeneralsMD\Code\GameEngineDevice\Source\W3DDevice\GameClient\W3DScene.cpp(174): warning C6011: Dereferencing NULL pointer 'TheWritableGlobalData'.

@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker Refactor Improves the structure of the code, with negligible changes in function. labels Jun 19, 2025
@xezon xezon force-pushed the xezon/staticanalysis2/017 branch from 3491b02 to 68bc384 Compare June 20, 2025 07:33
@roossienb
Copy link

While it works, this is not the right solution. An empty array is still a valid array (and can be accessed), while a null-pointer cannot be accessed.

There are no null-pointer checks when these arrays are being accessed, so a crash risk is introduced.

Imho the correct solution is to remove the NULL initialization of the arrays. Those are not needed, as they get overwritten by the array creation a few lines below.

@xezon
Copy link
Author

xezon commented Jun 21, 2025

Yes a 0 sized array is possible to allocate, but it is useless. It cannot be dereferenced.

My understanding of this code is that if the size was 0, then it would not be dereferencing the arrays.

Example:

RenderObjClass **occluderList=m_potentialOccluders; // Would be NULL
for (k=0; k<m_numPotentialOccluders; k++) // Would not enter the loop, because 0<0 is false.
{
	renderOneObject(rinfo, (*occluderList), localPlayerIndex);
	occluderList++;	//advance to next one
}

Can you show the code that you think would be unsafe if array size was 0?

Copy link

@roossienb roossienb left a comment

Choose a reason for hiding this comment

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

I checked and you are right. There are no places where this could be an issue

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 Improves the structure of the code, with negligible changes in function.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants