Skip to content

call yajl_gen_free() to prevent memory leaks #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mdavidsaver merged 1 commit into from
Jul 25, 2023
Merged

call yajl_gen_free() to prevent memory leaks #19

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions caPutLogApp/caPutJsonLogTask.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ void CaPutJsonLogTask::addPutToQueue(LOGDATA * plogData)
flag = call; \
if (flag != yajl_gen_status_ok) { \
errlogSevPrintf(errlogMinor, "caPutJsonLog: JSON generation error\n"); \
yajl_gen_free(handle); \
Copy link
Contributor

@simon-ess simon-ess Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@hinxx hinxx Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@mdavidsaver mdavidsaver Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@hinxx hinxx Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) \
...

Copy link
Contributor

@simon-ess simon-ess Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return caPutJsonLogError; \
} \
}
Expand Down Expand Up @@ -598,6 +599,7 @@ caPutJsonLogStatus CaPutJsonLogTask::buildJsonMsg(const VALUE *pold_value, const
/* First log to a PV so we can append new line later for the logging to a server */
this->logToPV(json);
this->logToServer(json.append("\n"));
yajl_gen_free(handle);
return caPutJsonLogSuccess;
}

Expand Down