[hdx] Be more defensive about picking and buffer bounds.

[marsupial]

Description of Change(s)

Clamp subRect passed to HdxPickResult to renderBuffer bounds.
Verify index into buffer is in its memory range.

Fixes Issue(s)

Possible out of bounds access in picking which will bring the application down.

[jtran56]

Filed as internal issue #USD-6058.

[tcauchois]

Hey Marsupial. Good find on the bounds checking here; it's absolutely something we should do.

This PR clamps "subrect" to the framebuffer bounds in HdxPickFromRenderbufferTask, does nothing for HdxPickTask, and then additionally adds a per-pixel bounds check to HdxPickResult.

I'm a bit worried about the per-pixel bounds check, since we have a few drag-select use cases where you loop over a large pixel range. Also, it's redundant with the clamping of subrect, for the case of HdxPickFromRenderBufferTask.

It seems like if you just clamped subrect, maybe in the HdxPickResult constructor, you wouldn't need to do the per-pixel check or modify the tasks. Would that work for your use case?

Thanks!
Tom

[marsupial]

The real issue (and I think why per-pixel needs to be in) is that in the event a buffer-write did not complete you can wind up with some heinous id-data giving those out of bounds access.

Has this/a per-pixel bounds check been measured as problematic? Given all that's going on in the pick-task I'd be really surprised if that was a bottleneck.

The clamping was a first attempt for a fix but wasn't sufficient (and I just left it in as it seemed more 'complete'). I can move it to the constructor tomorrow.

[tcauchois]

I'm unclear on what your comment means. GetPrimId (for example) is called from _ResolveHit, _GetHash, and _IsValidHit. These are all called from the various "HdxPickResult::Resolve*" functions. All of those functions, in turn, loop over _subRect.

If _bufferSize represents the amount of memory you have allocated, and _subRect is clamped to be within [0^2, bufferSize], any pixel chosen from _subRect will be within the buffer, and so the ID arrays will be safe to read from.

Can you give me a specific example where clamping to subRect didn't work?

Note that you'd need to add the clamping either once, in HdxPickResult, or twice, in HdxPickTask and HdxPickFromRenderbufferTask. If you only added it to HdxPickFromRenderbufferTask, and not the others, I'd expect it to still be broken for GL since GL doesn't use HdxPickFromRenderbufferTask (somewhat confusingly, sorry).

[marsupial]

I think you're right and was running into a separate issue with delegates & HdRenderPassState::GetViewport for "not-storm" delegates that I didn't separate.

I'll re-verify with the clamp-only fix.

[tcauchois]

One minor note about calculation of subrect[2]/[3], but otherwise this looks great and we'd love to accept it! Has it been working for all of your test cases?

Thanks for the iteration!

[tcauchois]

So the subrects are stored as [x,y,w,h], so I believe what you want is:
"sub.x > 0", "sub.x + sub.w < buffer.w". So this line should be:
_subRect[2] = std::min(_bufferSize[0] - _subRect[0], _subRect[2]);
... and likewise for _subRect[3].

Otherwise this looks great!

[marsupial]

Seems to be working well...

[c64kernal]

Looks good, thanks @marsupial