Page MenuHomePhabricator

efl_ui_item: protect against NULL access
AbandonedPublic

Authored by bu5hm4n on May 5 2019, 3:05 AM.

Details

Summary

if _mouse_down / _mouse_up is executed but select_mode is NULL, then
this will crash.

Depends on D8828

Diff Detail

Repository
rEFL core/efl
Branch
devs/bu5hm4n/work
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11295
bu5hm4n created this revision.May 5 2019, 3:05 AM
bu5hm4n requested review of this revision.May 5 2019, 3:05 AM
bu5hm4n updated this revision to Diff 21964.May 6 2019, 7:41 AM
bu5hm4n edited the summary of this revision. (Show Details)

Rebase & update according to xavi review

bu5hm4n updated this revision to Diff 22003.May 7 2019, 2:56 AM

Add zoomable to tests, and publish the required rebase, sorry Xavi :|

bu5hm4n updated this revision to Diff 22111.May 12 2019, 2:18 AM

Rebase & update

Did you encounter this problem? Or is this just in case?
Is it "legal" that select_mode is NULL?
And finally, wouldn't it be easier to check for NULL and exit earlier?

I just encountered this when i wrote the tests. I could return earlier. However i like to either sanity check that earlier on, or only return at the end. This ensures that the function either "early" returns, or returns at the end of the function. This makes the function less branchy, and very easy to understand when reading the code :).

And does it make sense that select_mode is NULL? I am just trying to make sure that we are not hiding a bigger problem.

I'd like to see more details about which widget this occurred in?

bu5hm4n added a comment.EditedMay 14 2019, 12:02 AM

This does not appear in any particular widget. This happens in the testcase where i am just instanciating efl_ui_item. I know this is meant to be inherited from. However, I wanted to test the base implementation, so i decided that this little fix here would be beneficial for us, since we can test & verify the implementation of efl_ui_item.

bu5hm4n updated this revision to Diff 22162.May 14 2019, 1:55 AM

Make press,move-out-of-widget,unpress scenarios work take 2

bu5hm4n updated this revision to Diff 22182.May 14 2019, 10:54 AM

update to new API names

bu5hm4n updated this revision to Diff 22194.May 14 2019, 11:22 AM

update to new API names, redo

zmike requested changes to this revision.May 14 2019, 12:48 PM

I'm not in favor of this kind of change since it makes the code misleading by implying that this is a scenario which can regularly occur. The test should either have its own item implementation with a constructor to handle this or efl.ui.item should have a base constructor to handle this. I'd prefer the former.

This revision now requires changes to proceed.May 14 2019, 12:48 PM
bu5hm4n abandoned this revision.May 15 2019, 6:36 AM

Lets solve this in some other way.