Fix aiGetMaterialProperty in the presence of material keys that are prefixes of other keys#6603
Fix aiGetMaterialProperty in the presence of material keys that are prefixes of other keys#6603Valakor wants to merge 2 commits into
Conversation
…refixes of other keys
📝 WalkthroughWalkthroughUpdated the key comparison in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Good catch, thanks for your input. I guess using strcpy will create some security flaws. |
|
The codebase seems to use |
…String can never be longer than this we'll either: 1. Finish comparing before AI_MAXLEN 2. Stop comparing after AI_MAXLEN and consider the strings "equal" even if "pKey" is longer This feels like a reasonable compromise to avoid malformed (non zero terminated) strings since I wouldn't expect a property name to be 1024 characters long.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/Material/MaterialSystem.cpp (1)
76-76: Fix correctly addresses the prefix-match bug.
prop->mKey.datais a fixedchar[AI_MAXLEN]buffer that is always NUL-terminated withinAI_MAXLENper theaiStringinvariants, andstrncmphalts at the first NUL in either operand. The comparison therefore (a) never reads past either string's NUL, and (b) correctly rejects prefix-only matches such as"$tex.mapping"vs"$tex.mappingname"because the differing byte at the shorter key's NUL position causes a mismatch. Even a hypotheticalpKeylonger thanAI_MAXLENis safely rejected, since the comparison terminates atprop->mKey.data's NUL.One optional consistency note: the other key comparisons in this same file (lines 354, 457, 494) use
strcmp, which is functionally equivalent here given theaiStringinvariants. Given the PR discussion already settled onstrncmp(..., AI_MAXLEN)as a defensive compromise, this is fine to leave as-is; just flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/Material/MaterialSystem.cpp` at line 76, The change using strncmp(prop->mKey.data, pKey, AI_MAXLEN) is correct and safe because aiString guarantees NUL-termination within AI_MAXLEN, so keep this comparison as-is (it prevents prefix-only matches like "$tex.mapping" vs "$tex.mappingname"). Ensure you retain the check that also validates type (UINT_MAX wildcard) in the same condition (refer to prop->mKey.data, pKey, AI_MAXLEN and the type/check for UINT_MAX). Optionally, for consistency you may change the other key comparisons that use strcmp (seen elsewhere in this file) to the same strncmp(..., AI_MAXLEN) pattern, but this is not required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/Material/MaterialSystem.cpp`:
- Line 76: The change using strncmp(prop->mKey.data, pKey, AI_MAXLEN) is correct
and safe because aiString guarantees NUL-termination within AI_MAXLEN, so keep
this comparison as-is (it prevents prefix-only matches like "$tex.mapping" vs
"$tex.mappingname"). Ensure you retain the check that also validates type
(UINT_MAX wildcard) in the same condition (refer to prop->mKey.data, pKey,
AI_MAXLEN and the type/check for UINT_MAX). Optionally, for consistency you may
change the other key comparisons that use strcmp (seen elsewhere in this file)
to the same strncmp(..., AI_MAXLEN) pattern, but this is not required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49f4431d-7699-42cf-b465-6264ded8fb92
📒 Files selected for processing (1)
code/Material/MaterialSystem.cpp
2 participants
The existing code compares each property key in a material to the input via
strncmp(prop->mKey.data, pKey, strlen(pKey)). This will incorrectly return longer keys whose prefix matches the searched-for key. For example: searching for"$tex.mapping"when the material contains"$tex.mappingname"will return the "mappingname" property despite that not being the requested key.I haven't done an exhaustive search of all material keys to find common prefixes, but I know this affects at least these two glTF properties that contain the common prefix
AI_MATKEY_MAPPING:AI_MATKEY_GLTF_MAPPINGNAMEandAI_MATKEY_GLTF_MAPPINGID.Summary by CodeRabbit