[Home]

Summary:ASTERISK-13703: [patch] func_odbc's OPT_ESCAPECOMMA's is undone by second ast_app_separate_args call when using Set(ARRAY...)
Reporter:Walter Doekes (wdoekes)Labels:
Date Opened:2009-03-06 07:20:33.000-0600Date Closed:2009-06-29 14:36:57
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Functions/func_odbc
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20090309__bug14614__2.diff.txt
( 1) 20090309__bug14614.diff.txt
( 2) double_set_unescape_workaround_for_func_odbc.osso-1.diff
( 3) double_set_unescape_workaround_for_func_odbc.osso-2.diff
( 4) double_set_unescape_workaround_for_func_odbc.osso-and-tilghman-1.diff
Description:When returning more than one column with func_odbc custom functions, the comma's that should separate the columns are not escaped. This happens when setting the variables through the ARRAY function:

=== Example ===

(extensions.conf)
exten => _X.,1,Set(ARRAY(a|b)=${MY_ODBC_FUNC()})

(func_odbc.conf)
read=SELECT 'column, with comma', 1

The above results in:
${a} == "column", ${b} == ", with comma"
instead of:
${a} == "column, with comma", ${b} == 1

=== Cause ===

ast_app_separate_args is called twice on
"""ARRAY(a|b)=value\, with comma,1"""
once in pbx_builtin_setvar() (pbx.c) and once in array() (func_strings.c) (through AST_NONSTANDARD_APP_ARGS).

The first one removes all the backslashes but doesn't split the arguments because the delimiter is '|'.

Then you have """value, with comma,1""" which is split in three parts.

=== Problem ===

The problem lies in the double unescaping of the backslash and has nothing to do with func_odbc per se. To set multiple variables at once, one must do:

exten => _X.,1,Set(ARRAY(a|b|c)=1\\|2\\|3)
(double escape)

=== Solution ===

I don't know if the double unescaping is intended (according to my O'Reilly book for 1.4 it isn't). When writing a Set statement without odbc functions, one can double escape the comma, or resort to multiple Set statements.

When using func_odbc, double-escaping the comma's could be a workaround. See the attached patch.
Comments:By: Walter Doekes (wdoekes) 2009-03-06 07:26:30.000-0600

For succesful use of the patch, one has to add "readarray=yes" to every read function in func_odbc.conf that returns more than one column.
By: Walter Doekes (wdoekes) 2009-03-06 08:08:00.000-0600

And.. for completeness sake: a different workaround is to (extra) escape the strings at SQL read time in func_odbc pending any fixing of Set(ARRAY..).

E.g., do:
read=SELECT REPLACE(REPLACE(my_string_column, '\\', '\\\\'), ',', '\\,'), 1
instead of:
read=SELECT my_string_column, 1
By: Tilghman Lesher (tilghman) 2009-03-08 20:43:59

Is there any reason why you WOULDN'T want to double escape the commas in all cases?
By: Walter Doekes (wdoekes) 2009-03-09 03:20:42

Well, yes. And here's why:

Set("SIP/1000-0820a2c0", "ARRAY(a|b)=bs-cm \\\\\\\, cm \\\, bs \\\\ eol,1") in new stack
NoOp("SIP/1000-0820a2c0", "a = bs-cm \, cm , bs \ eol") in new stack
NoOp("SIP/1000-0820a2c0", "b = 1") in new stack
Set("SIP/1000-0820a2c0", "a=bs-cm \\\\\\\, cm \\\, bs \\\\ eol") in new stack
NoOp("SIP/1000-0820a2c0", "a = bs-cm \\\, cm \, bs \\ eol") in new stack

If I set "readarray=yes" for both Set(ARRAY(a,b)=${MY_TWO_COLUMN_FUNC()}) Set(a=${MY_ONE_COLUMN_FUNC()}) then the latter one will get unescaped only once.


Preconditions for the above:
(1) It uses my patch and both MY*FUNC() obdc functions use "readarray=yes". I "fixed" my patch by adding a second """buf[buflen++] = '\\';""" in the readarray case. The patch was broken in that it only did a partial double escape.
(2) extensions.conf looks like this:
exten => _XXX.,n,Set(ARRAY(a,b)=${MY_TWO_COLUMN_FUNC()})
exten => _XXX.,n,NoOp(a = ${a})
exten => _XXX.,n,NoOp(b = ${b})
exten => _XXX.,n,Set(a=${MY_ONE_COLUMN_FUNC()})
exten => _XXX.,n,NoOp(a = ${a})
(3) The first column fetched from the database contains "bs-cm \, cm , bs \ eol"


If I set "readarray=no" in MY_ONE_COLUMN_FUNC, then the results turn out as wanted. As far as I could tell, the second unescape was caused by the ARRAY() function and that one is not called in the single column case. (Unless it is strictly forbidden to NOT use ARRAY when fetching odbc data...)
By: Walter Doekes (wdoekes) 2009-03-09 04:13:12

And yes, the [Problem] section in my initial report is wrong :)
My confusion was heightened by the different behaviour of the pipe and the comma: the pipe needs a double backslash, while the comma needs only one (although it copes fine with two, or even three).
By: Walter Doekes (wdoekes) 2009-03-09 04:22:23

double_set_unescape_workaround_for_func_odbc.osso-2.diff fixes the "partial escape" mentioned above and adds escaping of the pipe.
By: Tilghman Lesher (tilghman) 2009-03-09 12:42:43

I'd still like to avoid adding an option that would be unnecessary in 1.6.  I think I'd like to go with the option of setting an invisible variable with the values, unaffected by Set's parsing.  The trick is making sure that the value goes away at the right time, lest a literal setting of the ARRAY() function uses the wrong set of values.  Can you find a reasonable case where the attached patch that implements this approach won't work?  (BTW, this is modeled after the ODBCFIELDS approach in 1.6, which is used for the HASH() function.  The main difference is that there's no conflict between possible values in that usage -- there are field names passed invisibly -- or nothing.)
By: Tilghman Lesher (tilghman) 2009-03-09 16:12:28

I've made the patch even better.  Now it compares the invisible string with what was passed in and ensure that the passed in string, when escapes are compensated for, matches the invisible string.
By: Walter Doekes (wdoekes) 2009-03-10 08:24:44

I can agree that an extra temporary option is not the way to go. This ~ODBCVALUES~ workaround does not look like the cleanest of solutions, but I can live with it.

I've changed a couple of things:
- added escaping of double quotes and pipes
- removed some unecessary assignments

I'm not a 100% sure on whether your strecmp() is correct, but my testcase passes:
 source_data = """bs-cm \, cm , \" bs \ pp | eol"""", """1"""
 strecmp(
   """bs-cm \\\, cm \, \\\" bs \\ pp \| eol\",1""",
   """bs-cm \, cm , \" bs \ pp | eol",1"""
 ) == 0

If I understand correctly, things will only go wrong if you do an ODBC read and then later use ARRAY (without ODBC) with a string that looks very much like an unescaped version of the previously returned values -- which in turn is highly unlikely. Right?
By: Tilghman Lesher (tilghman) 2009-03-10 11:35:11

Could you provide me with a testcase of what your additional changes help with?  I'm unclear on that part.  (Specifically, a test case which fails with my patch and succeeds with yours.)

In terms of things going wrong, that's not possible with the latest patch.  The comparison avoids that possibility.
By: Walter Doekes (wdoekes) 2009-03-10 11:50:13

> Could you provide me with a testcase of what your additional changes help with?

Input:
"""bs-cm \, cm , bs \ dq " pp | eol"""

Extensions:
exten => _X.,n,Set(ARRAY(a,b)=${TEST_ARRAY()})
exten => _X.,n,NoOp(a = ${a})
exten => _X.,n,NoOp(b = ${b})
exten => _X.,n,Set(a=${TEST_SINGLE()})
exten => _X.,n,NoOp(a = ${a})

No patch:
Set("SIP/1000-0820a540", "ARRAY(a|b)=bs-cm \\\, cm \, bs \\ dq " pp | eol,1") in new stack
NoOp("SIP/1000-0820a540", "a = bs-cm , cm ") in new stack
NoOp("SIP/1000-0820a540", "b =  bs  dq  pp | eol") in new stack
Set("SIP/1000-0820a540", "a=bs-cm \\\, cm \, bs \\ dq " pp | eol") in new stack
NoOp("SIP/1000-0820a540", "a = bs-cm \, cm , bs \ dq  pp | eol") in new stack

Your patch:
Set("SIP/1000-0820a508", "ARRAY(a|b)=bs-cm \\\, cm \, bs \\ dq " pp | eol,1") in new stack
NoOp("SIP/1000-0820a508", "a = bs-cm \, cm , bs \ dq  pp | eol,1") in new stack
NoOp("SIP/1000-0820a508", "b = ") in new stack
Set("SIP/1000-0820a508", "a=bs-cm \\\, cm \, bs \\ dq " pp | eol") in new stack
NoOp("SIP/1000-0820a508", "a = bs-cm \, cm , bs \ dq  pp | eol") in new stack

My patch:
Set("SIP/1000-0820eb58", "ARRAY(a|b)=bs-cm \\\, cm \, bs \\ dq \" pp \| eol,1") in new stack
NoOp("SIP/1000-0820eb58", "a = bs-cm \, cm , bs \ dq " pp | eol") in new stack
NoOp("SIP/1000-0820eb58", "b = 1") in new stack
Set("SIP/1000-0820eb58", "a=bs-cm \\\, cm \, bs \\ dq \" pp \| eol") in new stack
NoOp("SIP/1000-0820eb58", "a = bs-cm \, cm , bs \ dq " pp | eol") in new stack


By: Walter Doekes (wdoekes) 2009-03-10 12:09:37

> In terms of things going wrong, that's not possible with the latest patch.
> The comparison avoids that possibility.

I wouldn't be to sure about that.

Input:
"""Doekes, Walter"""

Extensions:
exten => _X.,n,Set(ARRAY(a,b)=Doekes\, Walter)
exten => _X.,n,NoOp(a = ${a})
exten => _X.,n,NoOp(b = ${b})
exten => _X.,n,Set(irrelevant_var=${TEST_SINGLE()})
exten => _X.,n,Set(ARRAY(a,b)=Doekes\, Walter)
exten => _X.,n,NoOp(a = ${a})
exten => _X.,n,NoOp(b = ${b})

Your patch (and mine, for that matter):
Set("SIP/1000-0820a5e0", "ARRAY(a|b)=Doekes, Walter") in new stack
NoOp("SIP/1000-0820a5e0", "a = Doekes") in new stack
NoOp("SIP/1000-0820a5e0", "b =  Walter") in new stack
Set("SIP/1000-0820a5e0", "irrelevant_var=Doekes\, Walter") in new stack
Set("SIP/1000-0820a5e0", "ARRAY(a|b)=Doekes, Walter") in new stack
NoOp("SIP/1000-0820a5e0", "a = Doekes, Walter") in new stack
NoOp("SIP/1000-0820a5e0", "b = ") in new stack

In fact, this looks more serious than I thought.


By: Walter Doekes (wdoekes) 2009-03-17 02:42:36

For those that also have this issue, my current solution is as follows.

I create a MySQL function ASTERISK_ESCAPE and use that one to wrap all my columns (i.e. SELECT ASTERISK_ESCAPE(my_column), ASTERISK_ESCAPE(my_2nd_column)):

CREATE FUNCTION ASTERISK_ESCAPE (str VARCHAR(512)) RETURNS VARCHAR(512)
 RETURN REPLACE(REPLACE(REPLACE(REPLACE(str,
 '\\', '\\\\\\\\'), ',', '\\\\\\,'), '|', '\\\\\\|'), '"', '\\\\\\\\\\\\\\"');

This particular function only works in the following condition:
(1) You must set escapecommmas=no (the function purposefully double-escapes)
(2) It only works for multiple-column gets: Set(ARRAY(a,b,c)=${MY_ODBC_FUNC()}) (otherwise single escape would be appropriate)
(3) The double-quote is triple escaped. This is because I used the output as input to CALLERID(name) and that one doesn't escape double-quotes on my tested version (yielding invalid SIP headers or even header injection bugs).
By: Tilghman Lesher (tilghman) 2009-04-20 16:02:45

wdoekes: so I take it that you aren't actually using the C patch, correct?  You've found that it's more reliable if you do something within MySQL to compensate?
By: Digium Subversion (svnbot) 2009-04-20 17:02:18

Repository: asterisk
Revision: 189537

U   branches/1.4/funcs/func_odbc.c
U   branches/1.4/funcs/func_strings.c

------------------------------------------------------------------------
r189537 | tilghman | 2009-04-20 17:02:17 -0500 (Mon, 20 Apr 2009) | 11 lines

Add a workaround for func_odbc/ARRAY() for problems that occur with certain special characters.
In certain cases, due to the way Set() works in 1.4, values may not get set
properly.  This is a workaround for 1.4 only that corrects for these issues,
without making func_odbc more difficult to use properly.
(closes issue ASTERISK-13703)
Reported by: wdoekes
Patches:
      20090309__bug14614__2.diff.txt uploaded by tilghman (license 14)
      double_set_unescape_workaround_for_func_odbc.osso-and-tilghman-1.diff uploaded by wdoekes (license 717)
Tested by: wdoekes, tilghman

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=189537
By: Digium Subversion (svnbot) 2009-04-20 17:03:08

Repository: asterisk
Revision: 189538

_U  trunk/

------------------------------------------------------------------------
r189538 | tilghman | 2009-04-20 17:03:08 -0500 (Mon, 20 Apr 2009) | 17 lines

Blocked revisions 189537 via svnmerge

........
 r189537 | tilghman | 2009-04-20 17:02:16 -0500 (Mon, 20 Apr 2009) | 11 lines
 
 Add a workaround for func_odbc/ARRAY() for problems that occur with certain special characters.
 In certain cases, due to the way Set() works in 1.4, values may not get set
 properly.  This is a workaround for 1.4 only that corrects for these issues,
 without making func_odbc more difficult to use properly.
 (closes issue ASTERISK-13703)
  Reported by: wdoekes
  Patches:
        20090309__bug14614__2.diff.txt uploaded by tilghman (license 14)
        double_set_unescape_workaround_for_func_odbc.osso-and-tilghman-1.diff uploaded by wdoekes (license 717)
  Tested by: wdoekes, tilghman
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=189538
By: Walter Doekes (wdoekes) 2009-04-21 02:37:12

"""wdoekes: so I take it that you aren't actually using the C patch, correct? You've found that it's more reliable if you do something within MySQL to compensate?"""

That is correct.

Did you not read my comment at http://bugs.digium.com/view.php?id=14614#101480
Any patch involving """pbx_builtin_getvar_helper(chan, "~ODBCVALUES~"))""" breaks more than it fixes.

And since my original patch was rejected because it adds more option bloat, I'm using MySQL functions instead.



By: Tilghman Lesher (tilghman) 2009-04-21 10:06:16

I understand that there's only so much I can do to fix 1.4, due to the Multi-Set issues, but I'm doing the best I can.
By: Walter Doekes (wdoekes) 2009-04-21 10:49:40

Okay, you're the boss.
But I personally wouldn't recommend the new behaviour.

In the original case, the bug behaviour is deterministic.
In the new case, results vary depending on whether similar data (the single-column fetch) was loaded in the mean time, leading to bugs that are very hard to track down.

Please consider closing this as WONTFIX and reverting the patch.
By: Tilghman Lesher (tilghman) 2009-04-21 12:20:15

ARRAY() is not intended for use with non-func_odbc (or now, func_curl) cases, so I'm not really concerned with that behavior.  Yes, it's non-optimal, but I still think that Asterisk is better for having this patch.
By: Digium Subversion (svnbot) 2009-06-29 14:36:01

Repository: asterisk
Revision: 204170

U   branches/1.4/funcs/func_odbc.c
U   branches/1.4/funcs/func_strings.c

------------------------------------------------------------------------
r204170 | tilghman | 2009-06-29 14:36:01 -0500 (Mon, 29 Jun 2009) | 3 lines

Revision 189537 was supposed to make 1.4 more correct.  Instead, it broke func_odbc.  Reverting.
(closes issue ASTERISK-14177, issue ASTERISK-13703)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=204170
By: Digium Subversion (svnbot) 2009-06-29 14:36:57

Repository: asterisk
Revision: 204171

_U  trunk/

------------------------------------------------------------------------
r204171 | tilghman | 2009-06-29 14:36:57 -0500 (Mon, 29 Jun 2009) | 9 lines

Blocked revisions 204170 via svnmerge

........
 r204170 | tilghman | 2009-06-29 14:36:01 -0500 (Mon, 29 Jun 2009) | 3 lines
 
 Revision 189537 was supposed to make 1.4 more correct.  Instead, it broke func_odbc.  Reverting.
 (closes issue ASTERISK-14177, issue ASTERISK-13703)
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=204171