Page MenuHomePhabricator

elm_focus_legacy: Fix resource leaks
ClosedPublic

Authored by devilhorns on Thu, Mar 14, 10:14 AM.

Details

Summary

Coverity reports 2 resource leaks here because old_chain & new_chain
are never freed if we fail to return widget data. Use just
ELM_WIDGET_DATA_GET, manually check for valid 'ppd' and if false we
can just break out of the for loop here.

Fixes Coverity CID1399096, CID1399095

@fix

Depends on D8352

Diff Detail

Repository
rEFL core/efl
Branch
devs/devilhorns/coverity
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 10388
devilhorns created this revision.Thu, Mar 14, 10:14 AM
devilhorns requested review of this revision.Thu, Mar 14, 10:14 AM
cedric requested changes to this revision.Thu, Mar 14, 3:53 PM
cedric added inline comments.
src/lib/elementary/elm_focus_legacy.c
249

I think it would be best here to just continue instead.

This revision now requires changes to proceed.Thu, Mar 14, 3:53 PM
devilhorns added inline comments.Fri, Mar 15, 5:20 AM
src/lib/elementary/elm_focus_legacy.c
249

Uhhh, why ? The original code did a return ?? EINA_WIDGET_DATA_GET_OR_RETURN ....

devilhorns updated this revision to Diff 20618.Fri, Mar 15, 5:55 AM
devilhorns edited the summary of this revision. (Show Details)

rebase

devilhorns updated this revision to Diff 20625.Fri, Mar 15, 6:02 AM

no changes

bu5hm4n requested changes to this revision.Fri, Mar 15, 9:40 AM

@cedric I think this is right with returning. But a error would be better.

This revision now requires changes to proceed.Fri, Mar 15, 9:40 AM
devilhorns updated this revision to Diff 20640.Fri, Mar 15, 9:43 AM

Update patch based on review comments. Added ERR message.

cedric added inline comments.Fri, Mar 15, 9:48 AM
src/lib/elementary/elm_focus_legacy.c
249

I think the original code was a bit harder than it should have. I prefer the idea of recovering from an error instead of hard abort.

@cedric @bu5hm4n When you two decide which way you want this to go, let me know and I will update the patch accordingly :)

Yeah cedric is right the return; should probebly be break;

devilhorns updated this revision to Diff 20686.Mon, Mar 18, 5:16 AM
devilhorns edited the summary of this revision. (Show Details)

Update patch based on review comments. Instead of returning here, we

can now just break out of the for loop which will cleanup our

previously leaked variables.

bu5hm4n accepted this revision.Mon, Mar 18, 8:00 AM
devilhorns updated this revision to Diff 20695.Mon, Mar 18, 8:44 AM

no changes

cedric accepted this revision.Mon, Mar 18, 9:57 AM
This revision is now accepted and ready to land.Mon, Mar 18, 9:57 AM
devilhorns closed this revision.Mon, Mar 18, 11:47 AM

This has broken the build for me:

../src/lib/elementary/elm_focus_legacy.c: In function ‘elm_object_focus_next’:
../src/lib/elementary/elm_focus_legacy.c:248:19: warning: implicit declaration of function ‘ELM_WIDGET_DATA_GET’;