Outliner: Add missing check for the drag type in datastack_drop_poll
ClosedPublic
Actions

Authored by Robert Guetzkow (rjg) on Oct 11 2020, 12:34 PM.

Details

Reviewers

Julian Eisel (Severin)
Nathan Craddock (natecraddock)

Commits

rB8427e02abc5e: Fix T81589: Correct drag type handling in outliner

Summary

This is a fix for T81589. The datastack_drop_poll function did not include a check if the drag->type is WM_DRAG_ID. This resulted in the pointer drag->poin being interpreted as pointer to StackDropData even if it didn't have the correct type. The crash was caused by calling datastack_drop_init because the incorrect data was attempted to be used as pointer and dereferenced with drop_data->drag_tselem->type.

Adding the required type check appears to fix the issue.

Diff Detail

Repository

rB Blender

Branch

2020-10-11-outliner-drag-n-drop (branched from master)

Build Status

Buildable 10672
Build 10672: arc lint + arc unit

Event Timeline

Robert Guetzkow (rjg) requested review of this revision.Oct 11 2020, 12:34 PM

Robert Guetzkow (rjg) created this revision.

Harbormaster completed remote builds in B10672: Diff 29815.Oct 11 2020, 12:34 PM

I'm not sure if there is a case where drag could be NULL. If that is the case a check for this should be added as well, since this wasn't previously checked either.

Robert Guetzkow (rjg) edited the summary of this revision. (Show Details)Oct 11 2020, 12:38 PM

While this is acceptable as a hot-fix, I'd prefer actually adding a different drag-type for this, say WM_DRAG_DATASTACK. Otherwise, if another case is added that uses WM_DRAG_ID with the wmDrag.poin set, behavior of this code is undefined.

So WM_event_start_drag() should not use WM_DRAG_ID for the data-stack, or have an additional, non-ambiguous way to identify what data is set for wmDrag.poin.

This revision now requires changes to proceed.Oct 12 2020, 5:00 PM

@Julian Eisel (Severin) I fully agree. I didn't know enough about the design decisions behind the newly introduced feature, so I kept the initial patch limited to the bare minimum. I will update the patch sometime this week.

Separate drag type for outliner added

Harbormaster completed remote builds in B10691: Diff 29859.Oct 12 2020, 9:27 PM

Julian Eisel (Severin) added inline comments.Oct 12 2020, 11:09 PM

source/blender/editors/space_outliner/outliner_dragdrop.c
1370	This will break now for the cases where an actual ID is dragged. So for the latter of the two `if`s, it still has to be `WM_DRAG_ID`. You could change the drag type in the `if`-block if needed, but that seems ugly and can fail easily. Instead I'd suggest adding a `const bool use_datastack_drag = ELEM(...)` and setting the type based on that. And then just checking `if (use_datastack_drag)` below.