Page MenuHomePhabricator

Ecore_Evas_ews: add proper clean up when quit the func @fix
ClosedPublic

Authored by artem.popov on Jul 20 2016, 1:22 AM.

Details

Summary

add proper clean up when quit the func (as per Raster comment)

Diff Detail

Repository
rEFL core/efl
Branch
arcpatch-D4175
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2382
Build 2447: arc lint + arc unit
artem.popov updated this revision to Diff 9614.Jul 20 2016, 1:22 AM
artem.popov retitled this revision from to Ecore_Evas_ews: remove dead code.
artem.popov updated this object.
artem.popov edited the test plan for this revision. (Show Details)
artem.popov added a reviewer: cedric.
barbieri edited edge metadata.Jul 20 2016, 4:01 AM

In theory this can be true in out of memory case:

There is an engine, then strdup(engine) and it may return NULL

jpeg requested changes to this revision.Jul 25 2016, 2:02 AM
jpeg added a reviewer: jpeg.

@barbieri is right, this code is here for oom error handling (unlikely)

src/lib/ecore_evas/ecore_evas_ews.c
1326

this second line should instead be { free(_ews_engine); return EINA_FALSE); }

This revision now requires changes to proceed.Jul 25 2016, 2:02 AM
barbieri added inline comments.Jul 25 2016, 7:30 AM
src/lib/ecore_evas/ecore_evas_ews.c
1326

You missed to reset the pointer you free (_ews_engine), to null.

But you shouldn't worry with the free, this global will be released when the program finishes.

Maybe just force _ews_defaults_engine = EINA_TRUE. actually this can be done before the 2 "if"

raster added a subscriber: raster.Jul 28 2016, 7:30 PM

yeah this code isn't totally dead. if strdup fails... but on failure it will possibly leak tho and not clean up nicely.

it should more be line:

_ews_engine = engine ? strdup(engine) : NULL;
_ews_options = options ? strdup(options) : NULL;

if ((engine) && (!_ews_engine)) return EINA_FALSE;
if ((options) && (!_ews_options))
  {
     free(_ews_engine);
     _ews_engine = NULL;
     return EINA_FALSE;
  }
raster requested changes to this revision.Jul 28 2016, 7:33 PM
raster added a reviewer: raster.

Raster, while your approach is correct, these variables are cleared on shutdown (no leaks) and they are only used by the core if _ews_defaults_engine == EINA_FALSE, which is set after these checks.

then, to make it really safe, after you free the pointers, please enforce _ews_defaults_engine = EINA_TRUE.

_ews_engine = engine ? strdup(engine) : NULL;
_ews_options = options ? strdup(options) : NULL;

if ((engine) && (!_ews_engine)) return EINA_FALSE;
if ((options) && (!_ews_options))
  {
     free(_ews_engine);
     _ews_engine = NULL;
     _ews_defaults_engine = EINA_TRUE;
     return EINA_FALSE;
  }
artem.popov updated this revision to Diff 9681.Aug 1 2016, 10:49 AM
artem.popov edited edge metadata.
artem.popov retitled this revision from Ecore_Evas_ews: remove dead code to Ecore_Evas_ews: add proper clean up when quit the func @fix.
artem.popov updated this object.

Updating D4175: Ecore_Evas_ews: add proper clean up when quit the func

@fix

barbieri accepted this revision.Aug 2 2016, 10:09 PM
barbieri edited edge metadata.
cedric accepted this revision.Aug 19 2016, 3:40 PM
cedric edited edge metadata.
jpeg accepted this revision.Aug 21 2016, 9:18 PM
jpeg edited edge metadata.

@raster can you please accept and close this revision ?

raster accepted this revision.Sep 21 2016, 4:08 AM
raster edited edge metadata.
This revision is now accepted and ready to land.Sep 21 2016, 4:08 AM
raster closed this revision.Sep 21 2016, 4:09 AM