Page MenuHomePhabricator

elm_access: fix a potentional error of null deref
ClosedPublic

Authored by akanad on Aug 4 2020, 4:19 AM.

Details

Summary

this is a patch to fix a potentional error by null dereferencing.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
akanad created this revision.Aug 4 2020, 4:19 AM
akanad requested review of this revision.Aug 4 2020, 4:19 AM
akanad updated this revision to Diff 30962.Aug 4 2020, 4:37 AM
  • modify commit
akanad updated this revision to Diff 30966.Aug 4 2020, 4:43 AM
akanad edited the summary of this revision. (Show Details)
  • modify phab summary
bu5hm4n added a subscriber: bu5hm4n.Aug 4 2020, 5:34 AM

can you make a safety check out of that ? I dont think we should continue there if access data is missing.

akanad updated this revision to Diff 30997.Aug 4 2020, 11:41 PM
  • rebase
  • fix to exit rather than executing rest of the function
jsuya requested changes to this revision.Aug 4 2020, 11:52 PM
jsuya added reviewers: kimcinoo, bu5hm4n.
jsuya added a subscriber: kimcinoo.

If there is no target (info's next or prev), elm_access finds the target once again in the focus_relation (539 line).
(I think previous diff is right.)
@kimcinoo Please check this change.

This revision now requires changes to proceed.Aug 4 2020, 11:52 PM

If there is no target (info's next or prev), elm_access finds the target once again in the focus_relation (539 line).

I agree with that, but if info itself is NULL, then the object was not correctly registered, leading to the case that we are applying access API on a object that was not meant to be used, since its not registered, you would also not get the audios for leaving / entering it.

(I think previous diff is right.)
@kimcinoo Please check this change.

I am on @jsuya side.
If an object does not have access_info - btw, this could be possible if 'calloc' fails in _elm_access_object_register , or _elm_access_object_unregister is called -,
then access needs to use default order as access_info->next/prev is NULL for user.

Please refer to _elm_access_highlight_cycle as well.
If 'info' is NULL, then it is using focus stuff, not returning.

Mhm yeah, that is true. But on a conceptual level i am not sure if it is a good idea to do this, as there is no assertion the correct audio & visual are done, and thats kind of the point of the extension...

(Something else, shouldn't this API also return true when the fallback focus code is used ?)

bu5hm4n added a comment.EditedAug 5 2020, 12:41 AM

I guess this diff is not about the concept, i will just load the old version of it.

@bu5hm4n is also right. _access_highlight_next_get is called by elm_access_action.
If the elm_access_action is called, if elm_config->access is FALSE, then the elm_access_action should return FALSE.

Now I am on @bu5hm4n side.

Okay, i will wait with landing.

jsuya accepted this revision.Aug 5 2020, 1:01 AM

Ok I understand this. Thank you for review :)

This revision is now accepted and ready to land.Aug 5 2020, 1:01 AM
This revision was automatically updated to reflect the committed changes.

@kimcinoo should we adjust that behaviour in cycle ? (Same argument applies there)

@bu5hm4n to make efl_ui_win work, I would like to stay with current implementation.