Don't crash in UsdImagingInstanceAdapter::GetScenePrimPaths when passing invalid instanceIndices #1932
Conversation
|
Hi @sunyab / @tcauchois -- I can do a review pass on this before you take a look later this week, if that works. This fixes some critical crashes in Omniverse that rely on GetScenePrimPaths. I'll also look to get a developer on my team to look into making a unit test if possible. Thanks! |
|
Filed as internal issue #USD-7470 |
| // not a single index valid valid, make empty requestedIndicesMap | ||
| if (minIdx > maxIdx) | ||
| minIdx = maxIdx; | ||
|
|
There was a problem hiding this comment.
The comment says that requestedIndicesMap created below will be empty in this case, but it appears it will be a vector of size 1 instead.
But as a simplification: if there isn't a single valid index, can't we just return immediately?
There was a problem hiding this comment.
Yes, I updated the code to skip the whole loop if there's no valid instance index.
| if (instanceIndex < indices.size()) { | ||
| int remappedIndex = indices[instanceIndex]; | ||
| requestedIndicesMap[remappedIndex - minIdx] = i; | ||
| } | ||
| } | ||
|
|
||
| result.resize(instanceIndices.size()); |
There was a problem hiding this comment.
This resize call doesn't seem to take into account whether there were any invalid entries in instanceIndices, so in that case result will be too large and contain empty paths, which clients likely won't expect. Do we need to do some extra handling here?
There was a problem hiding this comment.
I changed the code to count the number of valid indices for a correct resize.
|
@asluk We're getting ready to publish 22.08-rc1 in the next few days and since this is a regression introduced in this release we'd like to merge this as soon as possible. I've left some initial review notes and will update if there are additional notes from the team. |
Thanks @sunyab ; I don't have any further notes on the PR on top of yours. |
…ing invalid instanceIndices
mtavenrath
force-pushed
the
get_scene_primpaths
branch
from
d00d2dc
to
63b2fb8
Compare
Description of Change(s)
Added index checking for each passed instance index to avoid out of bounds accesses.
Fixes Issue(s)
Crash when passing invalid instance indices