On 19/06/14 14:54, Stefan Schmidt wrote:
> Hello.
>
> On Thu, 2014-06-19 at 14:10, Tom Hacohen wrote:
>> On 19/06/14 13:49, Stefan Schmidt wrote:
>>> Hello.
>>>
>>> On Thu, 2014-06-19 at 12:13, Tom Hacohen wrote:
>>>> On 19/06/14 12:03, Stefan Schmidt wrote:
>>>>> Hello.
>>>>>
>>>>> I stumpled over this several times before but always forgot to bring
>>>>> it up. There are 7 coverity issues related to resource leaks for these
>>>>> two mentioned in the subject.
>>>>>
>>>>> CID 1193245 1193247 1194717 1191929 1191930 1191934 1191977
>>>>>
>>>>> Looking at the inline function and the macros shows that resources are
>>>>> allocated. What puzzles me now is who is in charge of freeeing them
>>>>> again? I can't see and macro for it and also from a data flow view I
>>>>> wonder where the free'ing should actually happen.
>>>>>
>>>>> Would be great if our EO experts could shed some light on this and fix
>>>>> it up.
>>>>>
>>>>
>>>> I honestly don't know why coverity is complaining. Those macros allocate
>>>> info, append to a list, and then the functions that call them should
>>>> return the lists for the caller to use. Clouseau is a good example user
>>>> of this API. Maybe there's something wrong in those specific coverity
>>>> instances, not sure. However the last time I looked everything was solid.
>>>
>>> Coverity is complaining about resources are allocated and going out of
>>> scope and thus leaking. Lets look at one example here to have
>>> something concrete.
>>>
>>> src/lib/elm_layout.c:1673
>>>
>>> EOLIAN static void
>>> _elm_layout_eo_base_dbg_info_get(Eo *eo_obj, Elm_Layout_Smart_Data *_pd
>>> EINA_UNUSED, Eo_Dbg_Info *root)
>>> {
>>> eo_do_super(eo_obj, MY_CLASS, eo_dbg_info_get(root));
>>> ELM_WIDGET_DATA_GET_OR_RETURN(eo_obj, wd);
>>>
>>> if (wd->resize_obj && eo_isa(wd->resize_obj, EDJE_OBJ_CLASS))
>>> {
>>> Eo_Dbg_Info *group = EO_DBG_INFO_LIST_APPEND(root, MY_CLASS_NAME);
>>> const char *file, *edje_group;
>>> Evas_Object *edje_obj = wd->resize_obj;
>>>
>>> eo_do(edje_obj, edje_obj_file_get(&file, &edje_group));
>>> EO_DBG_INFO_APPEND(group, "File", EINA_VALUE_TYPE_STRING, file);
>>> EO_DBG_INFO_APPEND(group, "Group", EINA_VALUE_TYPE_STRING,
>>> edje_group);
>>>
>>> Edje_Load_Error error = EDJE_LOAD_ERROR_GENERIC;
>>> eo_do(edje_obj, error = edje_obj_load_error_get());
>>> if (error != EDJE_LOAD_ERROR_NONE)
>>> {
>>> EO_DBG_INFO_APPEND(group, "Error", EINA_VALUE_TYPE_STRING,
>>> edje_load_error_str(error));
>>> }
>>> }
>>> }
>>>
>>> >From EO_DBG_INFO_LIST_APPEND we get *group which is allocated in the macro.
>>> The macro internally also appends it to the given list, in our case
>>> *root, if it exist.
>>>
>>> The follwoing EO_DBG_INFO_APPENDs only add more allocated values to
>>> *group but nobody handles the freeing when we reach the end of the
>>> function. Its also void so coverity thinks the allocated memory goes
>>> out of scope and is lost.
>>
>> As you said, it's added to the list. Sure there'll be a problem if root
>> is NULL, but it should never be. We can even remove the null checks from
>> the macro if it makes you feel safer about what's going on. :)
>>
>>>
>>> Given that the functions are not called at all in the code it seems
>>> that only a tool like Clouseau is using this "API" (Finding the
>>> symbols because the follow a naming convention?) and hopefully frees
>>> all resources by walking through them from *root over *group to the
>>> values.
>>
>> That's how it works with functions that return allocated data, the
>> caller has to free it.
>>
>>>
>>> It is totally clear to me why coverity is thinking here that its a
>>> resource leak as it sees memory allocated in a void functions and
>>> going out of scope without any code handling the free'ing. How would
>>> it know about other tools doing that.
>>>
>>> Is this actually how these functions are supposed to be used or did I
>>> took a wrong turn somewhere?
>>>
>>
>> This is how it's supposed to be use. We could potentially change it to
>> return root if it makes things more obvious in your PoV. It's just some
>> work I don't feel like doing if not necessary. Rest assured though, this
>> code works as expected (as seen with clouseau).
>
> Well, I still don't find the code very intuitive to follwo but not a
> big deal either.
>
> I will just mark these as false positives in Coverity and be done with
> it.
It's the same thing as eina_hash_add, eina_list_append and every other
similar thing.
--
Tom.
------------------------------------------------------------------------------
HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions
Find What Matters Most in Your Big Data with HPCC Systems
Open Source. Fast. Scalable. Simple. Ideal for Dirty Data.
Leverages Graph Analysis for Fast Processing & Easy Data Exploration
http://p.sf.net/sfu/hpccsystems
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
