[asterisk-dev] [Code Review] 3541: res_http_websocket: Create a websocket client

[wdoekes]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3541/#review11993
-----------------------------------------------------------


I didn't check all of it.


trunk/include/asterisk/http.h
<https://reviewboard.asterisk.org/r/3541/#comment21906>

    This is case sensitive,
    
    while ast_http_header_match is sensitive.
    
    I don't see that explained here.



trunk/main/http.c
<https://reviewboard.asterisk.org/r/3541/#comment21900>

    In the purely hypothetical case of HTTP/1.11 we'd have a mismatch without knowing it.
    
    if (strncmp(buf, version, size) || buf[size] != ' ') {



trunk/main/http.c
<https://reviewboard.asterisk.org/r/3541/#comment21901>

    Why are you checking the reason phrase? Is that really necessary?
    
    
    6.1.1 Status Code and Reason Phrase
    
       The Status-Code element is a 3-digit integer result code of the
       attempt to understand and satisfy the request. These codes are fully
       defined in section 10. The Reason-Phrase is intended to give a short
       textual description of the Status-Code. The Status-Code is intended
       for use by automata and the Reason-Phrase is intended for the human
       user. The client is not required to examine or display the Reason-
       Phrase.



trunk/main/http.c
<https://reviewboard.asterisk.org/r/3541/#comment21903>

    So a header of:
    
     " x : bla "
    
    turns into:
    
     name="x", value="bla "
    
    But RFC says the inverse:
    
     field-value = *( field-content | LWS )
     ^-- unlimited white on both sides
    
     field-name = token
     ^-- no white on either side



trunk/main/http.c
<https://reviewboard.asterisk.org/r/3541/#comment21905>

    For extra fun:
    
       Any LWS
       that occurs between field-content MAY be replaced with a single SP
       before interpreting the field value or forwarding the message
       downstream.
    
    Perhaps excess space should be trimmed in header_parse. That should fix issues for both header_match and header_match_in.



trunk/main/http.c
<https://reviewboard.asterisk.org/r/3541/#comment21907>

    Perhaps strcasestr instead of strstr?



trunk/main/uri.c
<https://reviewboard.asterisk.org/r/3541/#comment21908>

    If !param, shouldn't field be set to NULL? Or does ao2_alloc zero everything?



trunk/main/uri.c
<https://reviewboard.asterisk.org/r/3541/#comment21909>

    You check for uri->scheme, but not for strlen(uri->scheme) >= 1.
    
    If scheme is "", you're looking at who knows what.



trunk/main/uri.c
<https://reviewboard.asterisk.org/r/3541/#comment21910>

    Perhaps a /* SAFE */ comment behind that sprintf.



trunk/main/uri.c
<https://reviewboard.asterisk.org/r/3541/#comment21913>

    This is .. nasty. I understand that you didn't want to memmove and whatnot. But this looks awkwards.
    
    Perhaps something like this instead?
    
    tmp = ast_uri_copy_replace(res, NULL, NULL, NULL, 
        ast_uri_is_secure(res) ? secure_port : port,
        NULL, NULL);



trunk/main/uri.c
<https://reviewboard.asterisk.org/r/3541/#comment21914>

    memcpy?



trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/3541/#comment21916>

    +1 ?
    
    You're not adding a NUL.
    
    If we're being pedantic here, you should check that it gets (+sizeof(long)-1) so the memcpy below never overwrites the stack. (Depending on the value of CLIENT_KEY_SIZE%sizeof(long).)



trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/3541/#comment21915>

    calloc? You're writing to it very soon.



trunk/res/res_http_websocket.c
<https://reviewboard.asterisk.org/r/3541/#comment21902>

    You're ignoring line folding here.
    
    A comment that you're doing so would probably suffice.
    
    
    
       HTTP/1.1 header field values can be folded onto multiple lines if the
       continuation line begins with a space or horizontal tab. All linear
       white space, including folding, has the same semantics as SP. A
       recipient MAY replace any linear white space with a single SP before
       interpreting the field value or forwarding the message downstream.


- wdoekes


On May 28, 2014, 10:58 p.m., Kevin Harwell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3541/
> -----------------------------------------------------------
> 
> (Updated May 28, 2014, 10:58 p.m.)
> 
> 
> Review request for Asterisk Developers and Joshua Colp.
> 
> 
> Bugs: ASTERISK-23742
>     https://issues.asterisk.org/jira/browse/ASTERISK-23742
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Add client websocket capabilities to Asterisk.
> 
> 
> Diffs
> -----
> 
>   trunk/tests/test_websocket_client.c PRE-CREATION 
>   trunk/res/res_http_websocket.exports.in 414797 
>   trunk/res/res_http_websocket.c 414797 
>   trunk/main/uri.c PRE-CREATION 
>   trunk/main/http.c 414797 
>   trunk/include/asterisk/uri.h PRE-CREATION 
>   trunk/include/asterisk/http_websocket.h 414797 
>   trunk/include/asterisk/http.h 414797 
> 
> Diff: https://reviewboard.asterisk.org/r/3541/diff/
> 
> 
> Testing
> -------
> 
> Created some unit tests.
> 
> 
> Thanks,
> 
> Kevin Harwell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140529/504abac8/attachment-0001.html>