HPCC-33299: Fix ZAP file log filter leaks #19466
Conversation
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-33299 Jirabot Action Result: |
| @@ -4270,7 +4270,8 @@ void CWsWuFileHelper::readWULogToFiles(IConstWorkUnit *cwu, WsWuInfo &winfo, con | |||
| ForEach(*iter) | |||
| { | |||
| const char *processName = iter->query().queryProp("@podName"); | |||
| ILogAccessFilter *processLogFetchFilter = getBinaryLogAccessFilter(logFetchFilter, getPodLogAccessFilter(processName), LOGACCESS_FILTER_and); | |||
| Owned<ILogAccessFilter> podFilter = getPodLogAccessFilter(processName); | |||
| ILogAccessFilter *processLogFetchFilter = getBinaryLogAccessFilter(logFetchFilter, podFilter, LOGACCESS_FILTER_and); | |||
There was a problem hiding this comment.
Per our offline conversation, let's be explicit about pointer ownership for the sake of maintainability, this is a low throughput feature and we're ok accepting the added object creation overhead.
| @@ -260,7 +260,7 @@ struct WUComponentLogOptions | |||
| } | |||
|
|
|||
| if (componentsFilterObj != nullptr) | |||
| logFetchFilter = getBinaryLogAccessFilter(logFetchFilter, componentsFilterObj, LOGACCESS_FILTER_and); | |||
| logFetchFilter.setown(getBinaryLogAccessFilterOwn(logFetchFilter.getLink(), componentsFilterObj, LOGACCESS_FILTER_and)); | |||
There was a problem hiding this comment.
per offline convo: let's try to externalize the repeating logic into a function.
| * Ownership of the given reference is transferred to the function. | ||
| * @param operation The logical operation to be performed on the existing and new filters. | ||
| */ | ||
| void extendFetchFilter(Owned<ILogAccessFilter>& compoundFilter, ILogAccessFilter* extension, LogAccessFilterType operation) |
There was a problem hiding this comment.
we should expose this logic at the jlog.hpp level as compoundOwnedFilter(ownedfilter, extention, op)
- Own filter objects prior to throwing exceptions. - Ensure new filter object references are owned before they are linked in new compound filter objects, or released after they are linked. Signed-off-by: Tim Klemm <[email protected]>
timothyklemm
force-pushed
the
hpcc-33299-zap-filter-leaks
branch
from
c03c4a8 to
6892834
Compare
|
@ghalliday please merge. |
There was a problem hiding this comment.
@timothyklemm - please see comment re. ownership semantics.
Also, minor, but it looks like buildBinaryLogFilter in WsLogAccessService.cpp could also leak under the right circumstances, e.g. if leftFilter is created, but right is not and an exception is thrown. Would be good if the filters were Owned, then 'getBinaryLogAccessFilter' was called at the end (not getBinaryLogAccessFilterOwn'
| ILogAccessFilter *processLogFetchFilter = getBinaryLogAccessFilter(logFetchFilter, getPodLogAccessFilter(processName), LOGACCESS_FILTER_and); | ||
| zapLogFilterOptions.logFilter.logFetchOptions.setFilter(processLogFetchFilter); | ||
| Owned<ILogAccessFilter> processLogFetchFilter(logFetchFilter.getLink()); // retain original for next iteration | ||
| compoundOwnedFilter(processLogFetchFilter, getPodLogAccessFilter(processName), LOGACCESS_FILTER_and); |
There was a problem hiding this comment.
there's some unusual ownership semantics going on here. It's generally not good practice to pass in Owned objects and fill them in methods.
If I've read correctly, I'd expect this code to be something like:
Owned<ILogAccessFilter> podFilter = getPodLogAccessFilter(processName);
Owned<ILogAccessFilter> compoundFilter = getCompoundFilter(logFetchFilter, podFilter, LOGACCESS_FILTER_and);
zapLogFilterOptions.logFilter.logFetchOptions.setFilter(compoundFilter.getClear());
i.e. where the helper takes both filters, and returns a new one.
where getCompoundFilter is something like:
ILogAccessFilter *getCompoundFilter(ILogAccessFilter* arg1, ILogAccessFilter* arg2, LogAccessFilterType operation)
{
if (!arg1)
return LINK(arg2);
else if (!arg2)
return LINK(arg1);
else
return getBinaryLogAccessFilter(arg1, arg2, operation);
}
| @@ -326,15 +333,24 @@ struct WUComponentLogOptions | |||
| logFetchOptions.setStartFrom(logFilterReq.getLineStartFrom()); | |||
|
|
|||
| if (logFilterReq.getComponentsFilter().length() > 0) | |||
| compoundOwnedFilter(logFetchFilter, getOredComponentsLogFilter(logFilterReq.getComponentsFilter()), LOGACCESS_FILTER_and); | |||
| { | |||
| Owned<ILogAccessFilter> filterClause = getOredComponentsLogFilter(logFilterReq.getComponentsFilter()); | |||
There was a problem hiding this comment.
trivial: there's a mix of styles here assigning to Owned, here by assignment,
in other places via the ctor (e.g. line 274). Either fine, no real benefit of either, assignment is more typical.
Type of change:
Checklist:
Smoketest:
Testing: