Github user mike-jumper commented on a diff in the pull request:

    https://github.com/apache/guacamole-server/pull/183#discussion_r217132320
  
    --- Diff: src/libguac/parser.c ---
    @@ -222,7 +222,11 @@ int guac_parser_read(guac_parser* parser, guac_socket* 
socket, int usec_timeout)
                 retval = guac_socket_select(socket, usec_timeout);
                 if (retval <= 0)
                     return -1;
    -           
    +            
    +            /* Reset pointers if instruction buf len is less than max 
instruction len */
    +            if (buffer_end - unparsed_end < GUAC_INSTRUCTION_MAX_LENGTH)
    +                unparsed_end = unparsed_start = parser->__instructionbuf;
    --- End diff --
    
    Well, the good news is that your servlet is absolved. There can be no doubt 
that the protocol data is indeed reaching guacd correctly and is not being 
clobbered by shared use of a common socket as I originally suspected. [Your 
provided 
`write_log.txt`](https://issues.apache.org/jira/secure/attachment/12939404/write-log.txt)
 clearly demonstrates this:
    
    ```
    ...
    --- 23 bytes:
    00000000:  35 2E 6D 6F 75 73 65 2C 33 2E 35 34 31 2C 32 2E  5.mouse,3.541,2.
    00000010:  32 36 2C 31 2E 31 3B                             26,1.1;
    --- 23 bytes:
    00000000:  35 2E 6D 6F 75 73 65 2C 33 2E 35 34 32 2C 32 2E  5.mouse,3.542,2.
    00000010:  32 36 2C 31 2E 31 3B                             26,1.1;
    ```
    
    The bad news is that these changes are definitely incorrect, and they do 
indeed break the parser. The following are the blocks ultimately read by 
simple, sequential calls to `guac_socket_read()` within the parser according to 
[your provided 
`read.txt`](https://issues.apache.org/jira/secure/attachment/12939405/read-log.txt):
    
    ```
    ...
    --- 20 bytes:
    00000000:  35 2E 6D 6F 75 73 65 2C 33 2E 35 34 31 2C 32 2E  5.mouse,3.541,2.
    00000010:  32 36 2C 31                                      26,1
    --- 23 bytes:
    00000000:  35 2E 6D 6F 75 73 65 2C 33 2E 35 34 32 2C 32 2E  5.mouse,3.542,2.
    00000010:  32 36 2C 31 2E 31 3B                             26,1.1;
    ```
    
    Any changes which allow the parser to accept 
"5.mouse,3.541,2.26,15.mouse,3.542,2.26,1.1;" are incorrect, as that should 
result in a parse error. It is invalid protocol data.
    
    The issue is not that the parser is behaving incorrectly - it is correctly 
bailing out with a parse error when given invalid data. The issue is that bytes 
are somehow vanishing from the stream between otherwise normal calls to 
`read()`.


---

Reply via email to