AaronZhangL / unimrcp

Automatically exported from code.google.com/p/unimrcp
Apache License 2.0
0 stars 0 forks source link

VS debug version of the MRCP client test program crashes when attempting a GET-PARAMS #58

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Hi Arsen,

Here is an interesting problem. I do a GET-PARAMS on the follwing generic
headers:

MRCP/2.0 244 GET-PARAMS 9
Channel-Identifier: 1@speechrecog
Accept-Charset: 
Content-Base: 
Content-Location: 
Content-Length: 
Cache-Control: 
Logging-Tag: 
Vendor-Specific-Parameters: 
Fetch-Timeout: 
Set-Cookie: 
Set-Cookie2: 

The server returns the following response from which I will omit all but
one "Vendor-Specific-Parameters" line, because there are too many and they
do not matter for the problem at hand:

MRCP/2.0 3256 9 200 COMPLETE
Channel-Identifier: 1@speechrecog
Content-Base: 
Content-Location: 
Logging-Tag: 
Fetch-Timeout: 10000
Vendor-Specific-Parameters:
server.osrspeechrecog.result.mediatype=application/x-vnd.speechworks.emma+xml;st
rictconfidencelevel=1;mrcpv=2.06

.  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .(several lines were omitted)

Cache-Control: max-age=,min-fresh=0,max-stale=
Accept-Charset: UTF-8

You will notice that "Content-Length", "Set-Cookie" and "Set-Cookie2" are
absent from the response and the values for "Content-Base",
"Content-Location" and "Logging-Tag" are empty strings.

My program remembers the "id(s)" of the "properties" it requested and calls
mrcp_generic_header_property_check( message, id ) when the response message
was received from UniMRCP.

This works fine apparently if I build the program in "Release" mode.
mrcp_generic_header_property_check() returns FALSE for the missing headers
and returns empty strings in the member variables of type apt_str_t for the
headers which have empty values.

In "Debug" mode however, mrcp_generic_header_property_check() returns TRUE
for the GENERIC_HEADER_CONTENT_BASE id (for instance), so my code proceeds
in trying to get the value. But the "buf" member of
generic_header->content_base is 0 at this point and the program crashes. In
fact "buf" is 0 for all the member variables of the mrcp_generic_header_t
structure returned by mrcp_generic_header_get() which have the type
apt_str_t, except for those which have a non-empty value in the GET-PARAMS
response received from the server, that is "Cache-Control" and
"Accept-Charset". The value of "Fetch-Timeout" which is a number looks
correct, as well.

Could you try to test this behavior? On my machine it is mathematically
consistent.

The presence or absence of the long list of "Vendor-Specific-Parameters"
properties in the MRCP response doesn't seem to matter. The behavior
described above takes place even without the "Vendor-Specific-Parameters"
header in the request.

Thanks a lot.

Original issue reported on code.google.com by Curat...@gmail.com on 8 Jan 2010 at 4:47

GoogleCodeExporter commented 9 years ago
Of course, the same behavior occurred in 0.8.0 and 0.9.0.

Original comment by Curat...@gmail.com on 8 Jan 2010 at 4:49

GoogleCodeExporter commented 9 years ago
Hi Vlad,

I've tested the same using both debug and release builds and got consistent 
results.
In both cases mrcp_generic_header_property_check() returns TRUE for empty
GENERIC_HEADER_CONTENT_BASE header field. Though the value is indeed empty, so 
you
should check it first before use. For instance, check str.length > 0

At the moment I have no clue why mrcp_generic_header_property_check() could 
return
FALSE for the same case using release build. Would it be possible to add some 
traces
and see what is going wrong. Probably mrcp_header_parse() in 
mrcp_header_accessor.c
is a good starting point.

Original comment by achalo...@gmail.com on 8 Jan 2010 at 3:10

GoogleCodeExporter commented 9 years ago
Hi Arsen, 

Let me clarify this. I believe that mrcp_generic_header_property_check() 
returns the
 correct values in all cases (as far as I know). That is, it returns TRUE if a header
is present in the MRCP response and FALSE if it is not. This was not the issue.

The issue is that an API function should refrain from returning a NULL pointer 
there
where a char* is expected! There is a clear distinction between a (valid) 
pointer to
an empty character string and a NULL (zero value) pointer. Of course, the user
application can test the length of the string (which is available in this case) 
and
this is a minor change. But it is somehow an extra test which should not be 
required.
In fact, even for you it may be easier I guess if you do not interpret the 
parameter
values received from the server but pass them on transparently to the user
application. Or, if you really want to interpret them, and find that the value 
is
empty, just omit that parameter name from the message returned to the user. 

This is the way I would do it. As far as this "issue" it turns out that it is 
not an
issue at all but a matter of interpretation, so you can close it (I will, if I 
learn
how to do it).

A question though: do you think that it is correct that the server omitted in 
the
reply some headers which we requested in the GET-PARAMS request? I am talking 
about
"Content-Length", "Set-Cookie" and "Set-Cookie2"?

Thanks a lot for the help and have a nice weekend.

Original comment by Curat...@gmail.com on 9 Jan 2010 at 1:06

GoogleCodeExporter commented 9 years ago
Hi Vlad,

Well, now everything seems to be clear.

At first, lets discuss the empty header case. I agree, an empty string is more
preferable than NULL pointer from end user API perspective. So the question is 
how to
achieve this behavior. Allocation of an empty buffer, which would contain only 
'\0'
character for every empty apt_string, looks not so convenient to me. A good 
reference
example can be STL string. STL string doesn't allocate memory for an empty 
string. In
the meantime, string::c_str() function always returns a valid NIL terminated 
pointer.
So it's a typical encapsulation. Using pure see, it's not possible to hide "buf"
member from user space, but I still can provide accessors (helper functions) to 
use.
See r1390.
So apt_string_buffer_get() will always return a valid pointer.

Now a few notes concerning server behavior. The specification states
http://tools.ietf.org/html/draft-ietf-speechsc-mrcpv2-20#section-6.1.2

   If all of the headers requested are supported, the server MUST return
   a Response-Status of 200.  If some of the headers being retrieved are
   unsupported for the resource, the server MUST reject the request with
   a 403 Unsupported Header.  Such a response MUST include the (empty)
   unsupported headers exactly as they were sent from the client.

So the behavior of the server you use doesn't fully comply with the statement 
above.
Anyway, I think it's safe to assume, that missing headers aren't supported. It
shouldn't cause any issue, unless you was going to use those headers :)

Have a nice weekend.

Original comment by achalo...@gmail.com on 9 Jan 2010 at 6:36

GoogleCodeExporter commented 9 years ago
Got it, Arsen. Yes, the two new functions help the application look neater 
(although
the "ugliness" is transferred in the library).

Thanks for the other analysis. I may ask the provider, just for my curiosity.

Best regards. 

Original comment by Curat...@gmail.com on 10 Jan 2010 at 3:45