[ZBX-11204] remove_param() breaks double quote escaping Created: 2016 Sep 14  Updated: 2017 May 30  Resolved: 2016 Sep 26

Status: Closed
Project: ZABBIX BUGS AND ISSUES
Component/s: Server (S)
Affects Version/s: 2.2.15rc1, 3.0.5rc1, 3.2.1rc1, 3.4.0alpha1
Fix Version/s: 2.2.15rc1, 3.0.5rc1, 3.2.1rc1, 3.4.0alpha1

Type: Incident report Priority: Critical
Reporter: Glebs Ivanovskis (Inactive) Assignee: Unassigned
Resolution: Fixed Votes: 0
Labels: calculateditems, codequality, count, macrocontext, triggerfunctions
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

This function was completely broken in ZBXNEXT-300 (or has never been working correctly):

  • if it was asked to remove the first parameter it copies the first character of the second without checking whether it is double quote or not;
  • inside quoted parameters it skips '\' preceding double quote.

As a result, calculated item

count(dummy.echo[{$ECHO}],10m,"{$MACRO:\"with , and } characters\"}")

was simply counting values ignoring pattern.

Debug log wasn't very helpful:

  4807:20160914:142725.827 In get_value_calculated() key:'dummy.echo.count' expression:'count(dummy.echo[{$ECHO}],10m,"{$MACRO:\"with , and } characters\"}")'
  4807:20160914:142725.828 In calcitem_parse_expression() expression:'count(dummy.echo[{$ECHO}],10m,"{$MACRO:\"with , and } characters\"}")'
  4807:20160914:142725.828 calcitem_parse_expression() functionid:1 function:'count(dummy.echo[{$ECHO}],10m,"{$MACRO:\"with , and } characters\"}")'
  4807:20160914:142725.828 calcitem_parse_expression() expression:'{1}'
  4807:20160914:142725.828 In substitute_simple_macros() data:'{1}'
  4807:20160914:142725.828 End substitute_simple_macros() data:'{1}'
  4807:20160914:142725.828 End of calcitem_parse_expression():SUCCEED
  4807:20160914:142725.828 In calcitem_evaluate_expression()
  4807:20160914:142725.828 calcitem_evaluate_expression() function:'lld macro test:dummy.echo[{$ECHO}].count(10m,"{$MACRO:"with , and } characters"}")'
  4807:20160914:142725.828 In evaluate_function() function:'lld macro test:dummy.echo[{$ECHO}].count(10m,"{$MACRO:"with , and } characters"}")'
  4807:20160914:142725.828 In evaluate_COUNT()
  4807:20160914:142725.829 In __get_function_parameter_uint31() parameters:'10m,"{$MACRO:"with , and } characters"}"' Nparam:1
  4807:20160914:142725.829 In substitute_simple_macros() data:'10m'
  4807:20160914:142725.829 __get_function_parameter_uint31() flag:0 value:600
  4807:20160914:142725.829 End of __get_function_parameter_uint31():SUCCEED
  4807:20160914:142725.829 In zbx_vc_get_value_range() itemid:23934 value_type:4 seconds:600 count:0 timestamp:1473852445
  4807:20160914:142725.829 End of zbx_vc_get_value_range():SUCCEED count:20 cached:1
  4807:20160914:142725.829 End of evaluate_COUNT():SUCCEED
  4807:20160914:142725.829 End of evaluate_function():SUCCEED value:'20'
  4807:20160914:142725.829 End of calcitem_evaluate_expression():SUCCEED
  4807:20160914:142725.829 In evaluate() expression:'20'
  4807:20160914:142725.829 End of evaluate() value:20.000000
  4807:20160914:142725.829 get_value_calculated() value:20.000000
  4807:20160914:142725.829 End of get_value_calculated():SUCCEED


 Comments   
Comment by Sergejs Paskevics [ 2016 Sep 16 ]

Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-11204

Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 16 ]

(1) Your fix works, but introduces extra variable. I think it can be simplified.
Here is what I came up with:

void	remove_param(char *p, int num)
{
	int	quoted = 0;
	char	*buf;

	num--;

	for (buf = p; '\0' != *p; p++)
	{
		if (0 != num)
			*buf++ = *p;

		if (0 == quoted)
		{
			if (',' == *p)
				num--;
			else if ('"' == *p)
				quoted = 1;
		}
		else if ('"' == *p && '\\' != p[-1])
			quoted = 0;
	}

	*buf = '\0';
}

It's shorter, simpler and it's easier to comprehend (to my opinion ). What do you think?

glebs.ivanovskis As s.paskevics rightly pointed out, my version leaves comma if the last parameter is removed. Here is a new version:

void	remove_param(char *buf, int num)
{
	int	quoted = 0;
	char	*p;

	for (p = buf, num--; '\0' != *p; p++)
	{
		if (0 != num)
			*buf++ = *p;

		if (0 == quoted)
		{
			if (',' == *p)
				num--;
			else if ('"' == *p)
				quoted = 1;
		}
		else if ('"' == *p && '\\' != p[-1])
			quoted = 0;
	}

	/* terminate the string, overwrite comma if the last parameter was removed */
	*(0 == num ? --buf : buf) = '\0';
}

s.paskevics Looks good. RESOLVED in r62645.

glebs.ivanovskis Thanks! Good enough for a fix, I think. Please review my stylistic changes in r62682. Still RESOLVED.

glebs.ivanovskis Just realized that this implementation will fail to remove the first and the only parameter (and will write '\0' outside of array). We don't need such universal function, let's get rid of remove_param() and use slightly modified function_parse_param() and memcpy().
REOPENED

glebs.ivanovskis After a brief discussion we decided not to mess with 2.2 and get back to minimal r62560 fix.
CLOSED

Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 19 ]

(2) Common pattern in trigger function evaluation functions (like evaluate_FUNC()) is to check that num_param() isn't too high and then read the first parameter unconditionally. Should we add some protection there to catch situations when num_param() returns 0 for malformed parameter list?

s.paskevics You're rights. I added checking for minimal allowed number of parameters in trigger functions. RESOLVED in r62645.

glebs.ivanovskis num_param() is strange... For parameter-less function it returns 1 (meaning one empty parameter). So, validation for parameter-less functions must be a bit more sophisticated. Currently trigger

{host:key.diff()}=0

and diff(key) in calculated item formula fail.
And you've accidentally committed debug logging 

zabbix_log(LOG_LEVEL_ERR, "TEST4 %d", nparams);

REOPENED

glebs.ivanovskis Get back to r62560, we will do more cleanup of the code related to calculated items and trigger expressions in ZBX-11211.
CLOSED

Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 22 ]

(3) Please sync parameter names with header file.

s.paskevics RESOLVED in r62709.

glebs.ivanovskis Thank you! CLOSED

Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 22 ]

(4) It's safe to use p - 1 instead of prev because if we are already in "quoted" state this must be at least second character.

s.paskevics RESOLVED in r62709.

glebs.ivanovskis Nice! CLOSED

Comment by Glebs Ivanovskis (Inactive) [ 2016 Sep 23 ]

Successfully tested!

Comment by Sergejs Paskevics [ 2016 Sep 26 ]

Fixed in :

  • 2.2.15rc1 r62767.
  • 3.0.5rc1 r62768.
  • 3.2.1rc1 r62770.
  • pre3.3.0 (trunk) r62771.
Generated at Sat Apr 20 10:35:29 EEST 2024 using Jira 9.12.4#9120004-sha1:625303b708afdb767e17cb2838290c41888e9ff0.