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

(1) It would be better to add check for invalid key parameters earlier – in substitute_key_macros() function. This way the invalid macros in item key parameters would be detected in other places too.

However the changes are more complex. The following must be done:

1. Add SUCCEED/FAIL return value to quote_key_parameter() function. Fail if the parameter ends with '\' and it has to be quoted.
2. In Dbpatch_2010101() the quote_key_param() function return value must be checked and processed like other errors there.
3. Add SUCCEED/FAIL return value to the replace_key_param_f callback function.
   1. Move return value to output parameter and replace it with SUCCEED/FAIL return value in expression.c:replace_key_param() function. Fail if the quote_key_param() call fails.
   2. Move return value to output parameter and replace it with SUCCEED/FAIL return value in dbupgrade.c:replace_key_param() function. Fail if the quote_key_param() call fails.
4. Add SUCCEED/FAIL return value to str:replace_key_param() function. Fail if the callback function call fails.
5. Check for replace_key_param() return values in replace_key_params_dyn() function.

glebs.ivanovskis A hint. Something very similar was done in r60256 but was then reverted. Hope this helps.

viktors.tjarve RESOLVED in r60609.

wiper CLOSED

(2) sysinfo.c:quote_key_param()

	if (0 == forced && '\\' == (*param)[--sz_src])
		return FAIL;

If we check for not forced quoting here, then we will not check quoted parameters like "{$MACRO}". And we can't decrement sz_src here, as it is used later for coyping data. So it should be:

	if ('\\' == (*param)[sz_src - 1])
		return FAIL;

If we define macro:

{$A} = A,B$
{$B} = A,B\

then substitute key macros will:

• will not correctly expand key[{$A}]
• not fail with key["{$B}"]

viktors.tjarve RESOLVED in r60633.

wiper CLOSED

(3) expression.c:replace_key_param()

	if (NULL == strchr(data, '{'))
		return FAIL;

If key parameter does not have macro we must simply return SUCCEED without replacing data (as there is nothing to replace). Otherwise substitute_key_macros() will fail with key[ABC].

viktors.tjarve RESOLVED in r60633.

wiper CLOSED

(4) dbupgrade.c:DBpatch_2010101() - the error messages could be improved.
More specific error messages would be helpful:

• for param - unique description "\"%s\" contains invalid symbols and cannot be quoted
• for dsn - data source name "\"%s\" contains invalid symbols and cannot be quoted

Also I wondering why the following check was done in the original code:

if (1 != (nparam = num_param(param)))

After all quote_key_param(,0) will quote parameter only if it contains symbols that have to be quoted. But probably it's better to leave it.

viktors.tjarve RESOLVED in r60633.

wiper added more specific error messages.
RESOLVED in r60643

viktors.tjarve CLOSED

(5) dbupgrade.c:replace_key_param() should fail only if it failed to quote key parameter. It should return SUCCEED if there was nothing to quote.

viktors.tjarve RESOLVED in r60633.

wiper It still returned FAIL for the parameters that were not going to be updated:

if (1 != level || 4 != num)	/* the fourth parameter on first level should be updated */
		return FAIL;

This practically would cause the patch to fail for all items.
RESOLVED in r60644

viktors.tjarve CLOSED

(6) It would be more logical to free the allocated parameter if the quoting failed in both replace_key_param() functions (expression.c:replace_key_param() and dbupgrade.c:replace_key_param())

This way str.c:replace_key_param() could check only for NULL != param like it was before.

viktors.tjarve RESOLVED in r60633.

wiper CLOSED

Successfully tested. Please review changes in (4) and (5).

viktors.tjarve Reviewed and looks good.

Released in:

• 2.2.14rc1 r60652
• 3.0.4rc1 r60655
• 3.1.0 r60656

(7) Coverity found one more place in database upgrade patches where quote_key_param() is used (it was added in v3.0):

*** CID 149447:  Error handling issues  (CHECKED_RETURN)
/src/libs/zbxdbupgrade/dbupgrade_2050.c: 58 in DBpatch_2050001()
52      while (NULL != (row = DBfetch(result)))
53      {
54              char    *param, *oid_esc;
55              size_t  oid_offset = 0;
56
57              param = zbx_strdup(NULL, row[1]);
>>>     CID 149447:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "quote_key_param" without checking return value (as is done elsewhere 4 out of 5 times).
58              quote_key_param(&param, 0);
59
60              zbx_snprintf_alloc(&oid, &oid_alloc, &oid_offset, "discovery[{#SNMPVALUE},%s]", param);
61
62              /* 255 - ITEM_SNMP_OID_LEN */
63              if (255 < oid_offset && 255 < zbx_strlen_utf8(oid))

viktors.tjarve RESOLVED in r60678.

wiper The key quoting failure must be treated similartly to other conversion failures - log a warning and continue with the next item. See r60680

viktors.tjarve CLOSED

Released in:

• 3.0.4rc1 r60682
• 3.1.0 r60683

See (8) in ZBX-11223