Summary:	ASTERISK-14308: [patch] CUT() returns empty string for fields other than the 1st
Reporter:	David Chappell (chappell)	Labels:
Date Opened:	2009-06-12 13:31:59	Date Closed:	2009-06-18 13:29:53
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Functions/func_cut
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) cut_clarify.patch ( 1) cut_fix.patch ( 2) cut-0015320.patch
Description:	CUT() in 1.4.X works correctly.  In 1.6.X SVN it does not work due to an off-by-one error in the code which advances the pointer to the start of the range. ****** ADDITIONAL INFORMATION ****** I have marked the severity as "major" because it is a major regression in a commonly-used function and I can find no work-around.  The bug guidelines provide no guidance on how to assess severity. Here is a test case which demonstrates the problem: exten => 1156,1,Set(test=smith~tune) exten => 1156,n,NoOp(${CUT(test,~,1)})       ; works, returns "smith" exten => 1156,n,NoOp(${CUT(test,~,2)})       ; should return "tune", returns "" It appears that this bug was introduced because 1.4.X had some strange code which added 1 to a pointer which could very well be NULL and then reset it to NULL if the result was equal to NULL + 1.  Presumably this strange behavior bothered someone who was going around replacing calls to index() (used in 1.4.X) with calls to strchr().  The defective code leaves tmp2 pointing to the delimiter rather than the character after the delimiter. It took me a long time to figure this bug out because the implementation of the CUT() function is slightly obfuscated.  Many of the variables have names which say little or nothing about their purpose.  Examples are str, num1, and tmp2.  I had to rename them in order to understand the code. In my patch I: * Fix the bug by changing the tricky-to-understand call to strchr() with a call to strspn() (which make the buggy code consistent with the code which follows). * Rename variables to give them descriptive names. * Delay the allocation of an ast_str in order to eliminate de-allocation code in error handling. * Reorder some lines took keep separate operations separate. * Remove confusing usage of starting field number 0 for ranges such as -5.  Instead such a range is understood as 1-5.  This allows the elimination of several tests. * Made code to output delimiters after the first field more clear by changing the variable which controls it from a flag to a count of the number of fields already output. The resulting code passes the test which I described above.  It also passes the test described in bug 0015208.
Comments:	By: Tilghman Lesher (tilghman) 2009-06-15 12:49:22 1) I need the code clarification in a separate patch from the actual fix. 2) I need another patch for branches 1.6.0 through 1.6.2, because those versions do not have ast_str_substitute().  Additionally, in these patches, the fix itself should be the only part added. 3) You've removed several cases handled, where the field range allows the first or last numbers to be omitted.  You'll need to restore those cases for the patch to be acceptable. By: David Chappell (chappell) 2009-06-18 13:24:24 Response to tilghman's points: 1) I have attached new patches, cut_fix.patch and cut_clarify.patch.  They are against revision 200620. 2) I have downloaded and examined the latest 1.6.0 and 1.6.1 releases.  It appears to me that the change which introduced the bug was not committed to these branches. 3) Though at first glance at the patch it may look like I removed those cases, I did not.  What I did was change the names of the variables which indicate the start and end of the range to be extracted. I also changed the -N case to set num1 (now start_field) to 1 rather than 0.  Setting it to 0 served no purpose and required additional tests later on to compensate.  By fixing this off-by-one problem, I was able to eliminate unnecessary tests. By: Digium Subversion (svnbot) 2009-06-18 13:29:52 Repository: asterisk Revision: 201745 U   trunk/funcs/func_cut.c ------------------------------------------------------------------------ r201745 | tilghman | 2009-06-18 13:24:23 -0500 (Thu, 18 Jun 2009) | 7 lines Clarify CUT code, and in the process, fix a bug in trunk only (closes issue ASTERISK-14308) Reported by: chappell Patches:       cut_fix.patch uploaded by chappell (license 8)       cut_clarify.patch uploaded by chappell (license 8) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=201745