[Home]

Summary:ASTERISK-23679: [patch]CDR userfield merged incorrectly in ast_bridge_call
Reporter:Vitezslav Novy (vnovy)Labels:
Date Opened:2014-04-28 07:43:16Date Closed:2017-12-18 10:22:58.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:CDR/General Features
Versions:11.9.0 Frequency of
Occurrence		Constant
Related
Issues:
Environment:Attachments:( 0) 11-cdr_userfield.patch
( 1) 11-cdr_userfield2.patch
Description:In function ast_bridge_call (in main/features.c) CDR field of B leg is appended to  CDR userfield of A leg.
when at start userfield of leg A is 'aaa' and userfield of leg B is 'bbb' result in leg A userfield should be 'aaa;bbb'.
Because of incorrect merge result is 'aaaaaa;bbb'
Comments:By: Matt Jordan (mjordan) 2014-04-29 16:37:38.004-0500

I can't say that the behaviour in this patch isn't better than what exists, but there is a slight problem with it.

Technically, if I had a channel that was previously in a bridge with another channel, this patch blows away the userfield that was previously constructed. For example, same SIP/foo (with userfield foo) is bridged with SIP/bar (with userfield bar). I would expect the userfield on bar to be "foo;bar". If SIP/foo 'survives' the bridge (due to some dial flag), and enters into a bridge with SIP/yackity, the userfield would be "foo;bar;yackity". With this patch, however, it becomes "foo;yackity".

Now, the current code is still wrong - because as you pointed out, it actually concatenates the userfield portion of the first channel twice. Ew. But that doesn't mean getting rid of the concatenation is correct.

Different + CDRs is a dangerous thing.

This is one of those behaviours that is just "this is always how it's been" and was never really written down. So there's no canonical definition of the behaviour of userfield, The closest we've come is in [Asterisk 12|https://wiki.asterisk.org/wiki/display/AST/Asterisk+12+CDR+Specification], where we documented that the behaviour of the userfield is:

{quote}
A user defined field set on the channels. If set on both the Party A and Party B channel, the userfields of both are concatenated and separated by a ;
{quote}

This is now done in {{cdr.c}}, and will always concatenate just the two fields of the Party A and Party B channels. Because these fields are not on the channel themselves, there is no cumulative effect:

{noformat}
if (!ast_strlen_zero(it_cdr->party_b.userfield)) {
snprintf(cdr_copy->userfield, sizeof(cdr_copy->userfield), "%s;%s", it_cdr->party_a.userfield, it_cdr->party_b.userfield);
}
{noformat}

*tl;dr*: There's a bug here, but I'm not sure the patch is the safest way to fix it. If you went with appending the userfield but corrected the duplication of the first channel, that would be 'safe', in that the simplest case would be fixed and anyone depending on weird continuation of concatenation wouldn't be broken.
By: Vitezslav Novy (vnovy) 2014-04-30 03:54:54.110-0500

I don not think you are right in this : With this patch, however, it becomes "foo;yackity".

Correct value of concatenated userfied is created in tmp variable by sprintf.
No matter what was in A leg userfield, semicolon and B leg userfield is appended;
I do not see how 'bar' part from your example could disappear.
Only problem of the original code is that this correct value is appended to A leg userfield previous value.
It should be just set to A leg userfield.

Or am I overlooking something?

I believe this patch does not change anything about  CDR philosophy, it just try to fix incorrect manipulation with strings.


By: Matt Jordan (mjordan) 2014-04-30 16:21:01.551-0500

Yes, you're missing my point.

Your patch:
{noformat}
snprintf(tmp, sizeof(tmp), "%s;%s", ast_channel_cdr(chan)->userfield, ast_channel_cdr(peer)->userfield);
- ast_cdr_appenduserfield(chan, tmp);
+ ast_cdr_setuserfield(chan, tmp);
} else {
{noformat}

{{ast_cdr_setuserfield}} *destroys* what was on the channel. It doesn't append - it removes it. As I said, if the userfield on the channel was previously set because the channel was in a bridge with some other channel, you blow away that value instead of appending to it. That's a change in behaviour. Yes, the existing code is wrong - but yours changes the behaviour as opposed to just fixing the original bad append.
By: Vitezslav Novy (vnovy) 2014-05-03 03:27:24.077-0500

Another patch which appends semicolon and B-leg userfield to A-leg userfield without repeating A-leg userfield value.
By: Vitezslav Novy (vnovy) 2014-05-03 03:34:08.607-0500

I see now that not only top CDR record on stack is affected by ast_cdr_setuserfield or ast_cdr appenduserfield.
Is that your point?

I still believe first patch sets correctly userfield of top CDR record but I do not have any idea what can be in other CDR records on stack and what is logic of their updates.

I've inserted new patch which appends semicolon and B-leg userfield value to A-leg userfield. If it is also not correct then please just close the issue, because I do not have enough knowledge to solve the problem.

By: Rusty Newton (rnewton) 2014-05-09 09:46:37.416-0500

[~vnovy] , you probably want to go ahead and follow the [Code Review process|https://wiki.asterisk.org/wiki/display/AST/Code+Review] and get your patch on the [Reviewboard|https://wiki.asterisk.org/wiki/display/AST/Review+Board+Usage]. That way, Matt as well as others can provide peer review of the code through that tool.
By: Joshua C. Colp (jcolp) 2017-12-18 10:22:58.924-0600

I'm suspending this issue since CDRs have been rewritten while redoing bridging. If this is still applicable feel free to reopen with details using a supported version of Asterisk.