Aculab Client code unimrcp inter operability problem MRCP v1 about unimrcp HOT 32 CLOSED

Matthew this issue should be fixed in r741. I wonder if you can verify it on your
side with Aculab client.

Hi, will do. Currently looks like my Aculab client is broken because of system
upgrades and my sys manager is away skiing. Hope to verify the fix within a week or
so.

Matthew, have you got a chance to verify this issue?

Yes, apologies, I meant to add a comment.

The aculab MRCP v1 server compatibility test now passes. It doesn't work with v2 but
that as you pointed out 
that is probably a bug in their client.

I haven't tried a more sophisticated test as yet but the bug caused by splitting the
packets is fixed.

Really good news!
Now proceeding to MRCPv2 interop. As far as I remember, my assumption was Aculab
didn't support SIP-Trying and expected SIP-OK with SDP. If you are still interested
in and will be able to patch Sofia-SIP, probably I'll try to hack Sofia-SIP not to
send SIP-Trying. Just curious if it helps.

Sure, very happy to try that out.

Matthew,
I'm attaching the patched nta.c file, the only difference compared to the original
one can be found in nta.c.diff.
Please note the patch was made against Sofia-SIP 1.12.10 and it's just an ugly hack
which ignores all outgoing SIP-Trying responses.
Let me know if it helps.

Matthew, do you have any updates on this?

MRCPv1 interoperability has been tested and verified recently.

There is always the issue.

In my case, I received several messages AND the last message is incomplete (cf. line

1884 and 1937 in the attached file). I haven't the case where I receive only an 
incomplete message (but I can try to reproduce that)


I use v0.6.0 and a other client MRCPv1

I feel the problem occurs only in case start-line is incomplete/segmented. I'll try
to have a closer look later. Thanks.

Well, actually there is nothing wrong there, but the warning messages in the logs.
Lets analyze the first warning message containing "Cannot parse RTSP start-line"

TEARDOWN rtsp://192.168.1.29:1554/speechrecognizer RTSP/1.0
2009-05-20 15:30:22:240826 [WARN]   Cannot parse RTSP start-line

CSeq: 57
Session: 03b2887e46ef3d40

2009-05-20 15:30:23:068951 [NOTICE] Remove Session <03b2887e46ef3d40>

RTSP Message was only partially received, therefore parser originally failed to parse
it. However the message was successfully parsed and further processed as soon as
continuation data was received.
In other words, probably it makes sense to log warning messages only in case parser
finally fails, but not in case continuation is expected.

Ok, I'm going to make new test to validate

I found the issue in my case :

If you received an incomplete message like this :

---
TEARDOWN rtsp://192.168.1.29:1554/speechrecognizer RTSP/1.0<CRLF>
CSeq: 592<CRLF>
Session: 39f
---

The apt_text_header_read function does not detect the incomplete "Session:" header

and returns true.

The pair->value.buf contains the truncated "session id" and pair->value.length equals

0 (because no CR and/or LF).

Honestly I don't like what apt_text_header_read() returns now. At least it should
return TRUE only in case of valid headers, which end with CRLF and/or LF.
Even though overall processing still should work correct now, as rtsp_header_parse()
returns TRUE only in case the last header is empty. All the headers will be restarted
(parsed again) when the whole message will be finally received.

I made a little modification in the apt_text_header_read() to return TRUE only in case

of valid headers, which end with CRLF and/or LF to continue my test.

I have always an issue. Did you see another issue about that ? 

I'm watching the case when the stack receives only the start-line 

1. Enhancement of apt_text_header_read()
Yesterday I locally modified this stuff to return TRUE only in case header is indeed
valid according to our discussion, and I'll commit the modifications later today.
However it's just an enhancement and overall there should be no issue concerning
message segmentation even with the current trunk.
2. The remaining issue
If you still have an issue, I think it should not relate to message segmentation. Can
you describe what the remaining issue is? How do you observe it?

I don't understand why you say : "there should be no issue concerning message 
segmentation."

If we don't modify the apt_text_header_read() function to return TRUE only in case
of 
valids hearders :
    - which end with CRLF and/or LF
    - which contains at least one header

and If you receive an incomplete message which corresponds to the previous cases 
(easy to reprodruce with my client), the unimrcpserver server returns "Bad Request".

I re-ran my test with r940 and the modifications correct the "issue".

As I tell you before, I have another issue but I'm trying to find the sequence

I found the sequence :

You receive a SETUP in two steps as following :

1.

SETUP rtsp://192.168.1.29:1554/speechrecognizer RTSP/1.0<CR><LF>
CSeq: 10184<CR><LF>
Transport: RTP/AVP;unicast;client_port=54218-54219<CR><LF>
Content-Type: application/sdp<CR><LF>
Content-Length: 258<CR><LF>
<CR>

=> start-line OK
=> headers OK
=> "result == RTSP_STREAM_MESSAGE_TRUNCATED"


2. 

<LF>
v=0<CR><LF>
o=telisma 1243428652 1243428652 IN IP4 192.168.1.175<CR><LF>
s=telisma MRCP Client<CR><LF>
c=IN IP4 192.168.1.175<CR><LF>
t=0 0<CR><LF>
m=audio 54218 RTP/AVP 0 8 101<CR><LF>
a=rtpmap:0 pcmu/8000/1<CR><LF>
a=rtpmap:8 pcma/8000/1<CR><LF>
a=rtpmap:101 telephone-event/8000<CR><LF>
a=fmtp:101 0-15<CR><LF>
a=sendonly<CR><LF>
<CR><LF>

=> In the rtsp_parser_run, you process "continuation data" and the buffer contains
:

<LF>
v=0<CR><LF>
o=telisma 1243428652 1243428652 IN IP4 192.168.1.175<CR><LF>
s=telisma MRCP Client<CR><LF>
c=IN IP4 192.168.1.175<CR><LF>
t=0 0<CR><LF>
m=audio 54218 RTP/AVP 0 8 101<CR><LF>
a=rtpmap:0 pcmu/8000/1<CR><LF>
a=rtpmap:8 pcma/8000/1<CR><LF>
a=rtpmap:101 telephone-event/8000<CR><LF>
a=fmtp:101 0-15<CR><LF>
a=sendonly<CR><LF>
<CR><NUL>

=> With this buffer the sdp_session function in mrcp_unirtsp_sdp.c return NULL then

mrcp_descriptor_generate_by_sdp_session function generates a coredump

Lines are normally terminated by CRLF, but receivers should be prepared to interpret
CR and/or LF as line terminators too.
Thus
Content-Length: 258<CR><LF>
<CR>
=> start-line OK
=> headers OK
=> "result == RTSP_STREAM_MESSAGE_TRUNCATED"

looks correct, but the continuation <LF> is really fuzzy in this case. 
The only solution I see at the moment is to skip preceding <LF> from message content
/ if any.

Regarding the segfault: SDP parser may fail in several cases, and this cases should
be still processed gracefully. Must be fixed.

Reply to comment 10.
We have done some tests on aculab Prosody S.
Aculab mrcp1 --> unimrcp : working great
Aculab mrcp2 --> unimrcp : seems that unimrcp answer to the SDP invitation with a
Content-Length = 0 in the following SDP block.

Applying your patch fix the problem.

Do you know if this patch may have bad behaviour with other platform than aculab?
Is there an other way to be compatible with Aculab (except with a fix by aculab)?

Jan-Michel,
Thanks for the update.

Actually SIP TRYING contains no SDP and content-length of that message is always 
0. You confirm that my patch fixes the issue, it means Aculab indeed doesn't support

optional SIP TRYING and is expected SIP OK.
The patch I introduced is just a hack, which prevents Sofia sending TRYING. However

there is a better solution. Just recently someone has submitted another patch to 
Sofia's dev list which makes TRYING configurable from user space. Hopefully this 
patch will be included into Sofia's tree. Then I'll provide a configuration option
in 
unimrcpsever.xml to trigger this behviour.

From other hand, I hope Aculab sooner or later will support SIP TRYING to fully comply

with SIP RFC3261.

Here there are my modifications to support all uncomplete messages :

1. First character of the body part

 -> Suppress the CR or LF is exist

In the rtsp_message_body_read() function (rtsp_stream.c file)

---
apr_size_t required_length = message->header.content_length - message->body.length;

if (message->body.length == 0 && (*stream->pos == APT_TOKEN_LF || *stream->pos == 
APT_TOKEN_CR))
{
    stream->pos++;
    stream_length--;            
}

if(required_length > stream_length) {
---

2. Manage all uncomplete cases. I beleive your apt_text_stream_scroll() function is

not correct (I supposs ;) ):

-> New version of the apt_text_stream_scroll() function (apt_text_stream.c file)

APT_DECLARE(apt_bool_t) apt_text_stream_scroll(apt_text_stream_t *stream)
{
    apr_size_t scroll_length = (stream->text.length + stream->text.buf) - stream-
>pos;
    if(scroll_length == stream->text.length) {  
        stream->pos = stream->text.buf + scroll_length;
        return FALSE;
    }
    memmove(stream->text.buf,stream->pos,scroll_length);

    stream->text.length = scroll_length;
    stream->text.buf[scroll_length]='\0';
    stream->pos = stream->text.buf + scroll_length;
    return TRUE;
}

-> return of the apt_text_stream_scroll() function (rstp_stream.c file)

if(apt_text_stream_scroll(stream) == TRUE) {
    apt_log(APT_LOG_MARK,APT_PRIO_INFO,"Scroll RTSP Stream TRUE [%d]",stream->pos 
- stream->text.buf);
}
else {  
    // do nothing
}

Anthony, thanks for considering not only reporting issues, but also trying to fix them.

1. Actually <CR> | <LF> segmentation may occur not only between header and body, but
also between header fields or even between messages which contain no body. I'll try
to completely address these cases during the days.
2. I consider apt_text_stream_scroll() doing what it's intended to do: scroll buffer,
prepare for the next read/parse in case message is only partially parsed with current
attempt.

Arsen,

1. My current test allows to test all segmentations (the SETUP length is 248 bytes
in 
my test, then I run sequentially 248 calls with all the possible segmentations. I 
also run a test with several call and random segmentation).

2. About the apt_text_stream_scroll() function, I beleive to understand what it's 
intended to do but are you sure about your memmove "definition" ? This C function 
does not move (or scroll) memory. It's only make a copy of n bytes with overlap 
protection. The third parameter (of the memmove) must be the lentgh of the bytes to

move (but not the length of the bytes to scroll).

I hope to be wake up when I say that ;)

Anthony,

1. Just great, however I'm afraid only one case still remains
TEARDOWN rtsp://192.168.0.1:1554/media/speechsynthesizer RTSP/1.0
CSeq: 3
Session: 50ec40b84b3efb49
<CR>
---segmentation---
<LF>TEARDOWN rtsp://192.168.0.1:1554/media/speechsynthesizer RTSP/1.0
CSeq: 3
Session: 50ec40b84b3efb98
<CR><LF>
The probability it happens is less than 0.01%, but it's still possible.
I'll try to address it later today.

2. My apologies, you are completely right here. I could be blind or just a bit tired.
Thanks for the catch. Fixed in the trunk.

Anthony, 
1. Done in r962, please have a look and run your test cases again if needed.

Arsen,

2. There is always an issue :

your code :

if(!remaining_length || remaining_length == stream->text.length) {
    stream->pos = stream->text.buf + remaining_length;
    return FALSE;
}

I believe the good solution (tested) is :

if(!remaining_length || remaining_length == stream->text.length) {
    stream->pos = stream->text.buf + remaining_length;
    return FALSE;
}

or

if(remaining_length == stream->text.length) {
    stream->pos = stream->text.buf + remaining_length;
    return FALSE;
}

because if (remaining_length  == 0) then (remaining_length == stream->text.length)

Anthony,
2. Hopefully finally fixed in r963. Thanks!