call yajl_gen_free() to prevent memory leaks

[hinxx]

Running valgrind --leak-check=full --show-leak-kinds=all caPutJsonLogTest it was observed:

==2145056== 29,216 (352 direct, 28,864 indirect) bytes in 11 blocks are definitely lost in loss record 833 of 840
==2145056==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==2145056==    by 0x23A4A2: yajl_buf_alloc (yajl_buf.c:58)
==2145056==    by 0x23A95B: yajl_gen_alloc (yajl_gen.c:123)
==2145056==    by 0x16F977: CaPutJsonLogTask::buildJsonMsg(VALUE const*, LOGDATA const*, bool, VALUE const*, VALUE const*) (caPutJsonLogTask.cpp:399)
==2145056==    by 0x1706B3: CaPutJsonLogTask::caPutJsonLogTask(void*) (caPutJsonLogTask.cpp:301)
==2145056==    by 0x23012C: start_routine (osdThread.c:441)
==2145056==    by 0x4874608: start_thread (pthread_create.c:477)
==2145056==    by 0x4D52132: clone (clone.S:95)

After this PR change was applied valgrind did not report and more leaks from CaPutJsonLogTask::buildJsonMsg.
Tested with epics base 7.0.7.

[anjohnson]

Thanks for finding and fixing these, I agree with your changes. I will merge this PR (and the others now waiting) as soon as I get a chance.

[simon-ess]

One minor comment here: this seems weird in the macro to reference handle which is not in fact passed to the macro.

Admittedly I don't know of an easy way to fix it; the call parameter can be functions with variable numbers of arguments. But this looks a bit weird to have a reference in a macro that isn't actually passed to the macro.

[hinxx]

IMHO, not a big deal, I've seen code do that.
The macro will fail at compile time if used in function that does not specify 'handle'.
OTOH, moving the macro into the function is also an option; this way it "signals" to the devs that it is "meant" to be used in that function only (macro will nevertheless remain defined even outside the function).

[mdavidsaver]

In c++ the conventional way to avoid this sort of resource leak would be to wrap the handle in a class with yajl_gen_free() in a destructor. eg.

https://github.com/epics-base/pvDataCPP/blob/2547514abb90875bf47dda728ecaa9e30a5ab354/src/json/parseinto.cpp#L289-L300

https://github.com/epics-base/pvDataCPP/blob/2547514abb90875bf47dda728ecaa9e30a5ab354/src/json/parseinto.cpp#L329-L335

[hinxx]

In c++ the conventional way to avoid this sort of resource leak would be to wrap the handle in a class with yajl_gen_free() in a destructor. eg.

That looks clean and safe. Since this code has no such wrapper class at the moment is it worth introducing it now for purpose at hand? Maybe.

If the ugliness of accessing handle from the macro is what we are solving, wouldn't passing it in as the first arg to the macro be acceptable?

#define CALL_YAJL_FUNCTION_AND_CHECK_STATUS(handle, flag, call) \
...

[simon-ess]

I think that @mdavidsaver 's solution seems nicer to me; it's a pretty thin wrapper class to guarantee that we are going to fix this and other potential memory issues. Passing the handle to the macro as well feels more like a bandaid.

[mdavidsaver]

It was not initially clear to me from reading this thread, but from listening to @ttkorhonen during the recent collaboration meeting I have gathered that the leak being plugged here is on the "success" path, and is not limited to caPutJsonLogTest. It was originally detected "in the wild" as leaks in an IOC each time a message was logged as JSON. While I would certainly like to see "proper" C++ design conventions applied to this code, the severity of this bug imo. would merit releasing a "temporary" fix.

@anjohnson Are you the one to merge this PR? (as with the original #4)

[anjohnson]

Yes, and I would merge #15 as well since it prevents seg-faults. #16 and #17 add extra functionality that I haven't tried out myself yet, and in the absence of other reviews I would probably hold off on merging them right now — do you agree?

[mdavidsaver]

... do you agree?

Maybe not? I don't see a reason to hold up plugging this runtime memory leak while waiting on feature PRs. I think it would be nice if someone could come back and make this .cpp file look more like c++ code, but again not a good reason to wait.

It appears that I have permission to apply this fix, so I will.