Skip to content

Update CScriptNetPropManager#541

Open
samisalreadytaken wants to merge 5 commits into
mapbase-source:developfrom
samisalreadytaken:netprops
Open

Update CScriptNetPropManager#541
samisalreadytaken wants to merge 5 commits into
mapbase-source:developfrom
samisalreadytaken:netprops

Conversation

@samisalreadytaken

Copy link
Copy Markdown

The warning message is quite important, it's easy to make typos and not be aware of it ever.


PR Checklist

  • My PR follows all guidelines in the CONTRIBUTING.md file
  • My PR targets a develop branch OR targets another branch with a specific goal in mind

@azzyr azzyr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks good to me otherwise, i'll close #535

@@ -1249,19 +1289,21 @@ class CScriptNetPropManager
if ( pInfo->datatype == types::_VEC3 )
arraysize *= 3;

if ( index < 0 || (unsigned int)index >= arraysize )
if ( (unsigned int)index >= arraysize )

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is there a particular reason for removing these lower bound checks? index is provided by the user so this seems scary

@samisalreadytaken samisalreadytaken Jun 4, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Unsigned cast of all negative numbers ([-1, -2147483648] -> [2147483648, 4294967295]) is always larger than the limit. It compiles into same code with less instructions (0-test) because arraysize is always non-zero.

If, on 64-bit, this function were to take a 64-bit signed index and this unsigned cast stayed 32-bit, values in the range [0x8000000000000000, 0x800000007fffffff] would truncate the sign bit and access out of bounds, but that's a lot of unlikely ifs.

I added some more assertions and a comment to inform future editors. This isn't a super important change though, impact of these 2 instructions is negligible

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

Successfully merging this pull request may close these issues.

2 participants