Fix the generation of a header from multiple headers.

Logged In: YES
user_id=1759384
Originator: YES

Forgot to tell, this patch is for the release 2.0.1.

Logged In: YES
user_id=1475960
Originator: NO

Hello,

This fix has already been commited in the stable SVN branch and will be included in the next stable release.

Olivier Boulkroune

Logged In: YES
user_id=1759384
Originator: YES

Hi Olivier,

I just looked at the revision 239 of the file call.cpp. I think my patch still has some value, because:
1. The fix in latest call.cpp will not handle correctly the situation when the first header is empty for whatever reason. May be it is not possible in practice, but just looking at the function code (http://sipp.svn.sourceforge.net/viewvc/sipp/sipp/branches/2_0/call.cpp?revision=239&view=markup, the lines 1177-1182), if last_header array has the first byte '\n' followed by '\0' and the dest pointer points to the '\0', then the first '\n' character will not be removed.
2. Look at lines 1188-1190. The line 1189 src += strlen(name); should not be under the condition if (!content). It should always be executed. You do not ever want the name of the header in the middle of the header value, do you?

The patch that I propose handles both these cases well. Please review it and apply if possible.

Regards,
Anatoly.

Logged In: YES
user_id=1475960
Originator: NO

Anatoli,

I attract your attention to the following patch : https://sourceforge.net/tracker/?func=detail&atid=637566&aid=1748790&group_id=104305

, which seems to be incompatible with yours. What do you think about it ?

Fixed patch for call.cpp file.

Logged In: YES
user_id=1759384
Originator: YES

Hi Olivier,

the truth is, both my patch and the patch 1748790 are not 100% correct. My patch does not work correctly when content==true and patch 1748790 does not work correctly when content==false. I can provide extended explanations why both my patch and patch 1748790 are not 100% correct, please let me know if I should do this. I am attaching a new patch now that will work correctly in both those cases.

Regards,
Anatoly

File Added: call.cpp.diff

Logged In: YES
user_id=1475960
Originator: NO

Hi Anatoli,

Yes, further explanations are welcome, as I did not have time yet to go deeper with the understanding of this.

Thanks and regards,

Olivier Boulkroune

Logged In: YES
user_id=1759384
Originator: YES

Hi Olivier,

I started to write a long explanations. But then I realized that I do not understand how the problem of headers clipping that the patch 1748790 is supposedly fixes can happen. As a matter of fact, my patch, before I fixed it, could cause this problem. But the original version of the call.cpp file should not cause this problem. I asked the author of the patch 1748790 to provide some more explanations.

Regards,
Anatoly

Logged In: YES
user_id=1759384
Originator: YES

File Added: get_header_ut.cpp

Unit Test program for get_header function.

Test data

Logged In: YES
user_id=1759384
Originator: YES

File Added: data1.txt

Logged In: YES
user_id=1759384
Originator: YES

Hi Olivier,

the author of the patch 1748790 does not answer my comments. To get something going with these patches, I created a unit test program that tests different versions of the call:get_header function. It tests the version of the function that is in 2.0.1, the version with this patch 1733760 applied, the version with the patch 1748790 applied, and the version from the last revision of the call.cpp file downloaded from the subversion repository. You can run this program under a debugger to see how the different versions of the call_header function work. I also attached a test data that I used with the test program. Below is the output of the commands that I ran with interpretation:

$ g++ get_header_ut.cpp

$ ./a.out Via: 0 < data1.txt
Result, release 2.0.1: "Via: SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5., SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5., SIP/2.0/UDP 10.104.49.186:42000;rport=42000"
Result, patch 1733760: "Via: SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.0, SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.0, SIP/2.0/UDP 10.104.49.186:42000;rport=42000"
Result, patch 1748790: "Via: SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.,Via: SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.,Via: SIP/2.0/UDP 10.104.49.186:42000;rport=42000"
Result, revisiton 239: "Via: SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.0, SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.0, SIP/2.0/UDP 10.104.49.186:42000;rport=42000"

Here I tested getting the Via: header with content=false. As you can see, the versions 2.0.1 and patch 1748790 failed the test, only this patch and the last revision are fine.

$ ./a.out Via: 1 < data1.txt
Result, release 2.0.1: "SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5., SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5., SIP/2.0/UDP 10.104.49.186:42000;rport=42000"
Result, patch 1733760: "SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.0, SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.0, SIP/2.0/UDP 10.104.49.186:42000;rport=42000"
Result, patch 1748790: "SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5., SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5., SIP/2.0/UDP 10.104.49.186:42000;rport=42000"
Result, revisiton 239: "SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.0, SIP/2.0/UDP xxx.xxx.xxx.xxx;branch=z9hG4bKab16.0089d3d5.0, SIP/2.0/UDP 10.104.49.186:42000;rport=42000"

The same test with content=true. Again, only this patch and the last revision of the file are fine.

$ ./a.out X-Test: 0 < data1.txt
Result, release 2.0.1: "X-Test, , b"
Result, patch 1733760: "X-Test:, a, b"
Result, patch 1748790: "X-Test,X-Test: ,X-Test: b"
Result, revisiton 239: "X-Test:, a, b"

The test with X-Test: header and content=false, and the same result. This patch and revision 239 are OK, release 2.0.1 and patch 1748790 are not OK.

$ ./a.out X-Test: 1 < data1.txt
Result, release 2.0.1: ", , b"
Result, patch 1733760: ", a, b"
Result, patch 1748790: ", , b"
, a, b" revisiton 239: "

On this test, only this patch gives the correct result. Well, I can agree that it is unlikely that in practice there will be a message with multiple instances of the same header and with the first instance empty. So, the last revision of the call.cpp file is not bad at all. But why not make it perfect?

About the patch 1748790, may be the author was not using the original version of the call.cpp file from the release 2.0.1. I do not think that the call::get_header function in 2.0.1 can cause the problem that he described. Actually, the patch that I attached here originally (before the fix) can cause the problem that he described. We are all humans and make mistakes. But the results of this unit test program make me reasonably sure that my proposed patch is good now.

Best regards,

Anatoly.

Logged In: YES
user_id=1475960
Originator: NO

Checked-in (rev 284).

Thanks a lot Anatoly !

Regards,
Olivier