parseAll() hangs if an invalid line is received

[Renegade334]

If a string containing a non-RFC-compliant line is passed to parseAll(), it gets rejected by parse() and this breaks the parseAll() loop, on the assumption that the buffer does not yet contain a complete line. This causes the client to hang.

For example:

$buf = implode('', [
    ":server.name 375 BotNick :server.name message of the day\r\n", // valid line
    ":server.name 372 BotNick :Who left a carriage return \r in here?\r\n", // invalid line
    ":server.name 376 BotNick :End of message of the day.\r\n", // valid line
    ":server.name 251 BotNick :There are", // incomplete line
]);

$parsed = $myParser->consumeAll($buf);
echo str_replace("\r", '', var_export(['$buf' => $buf, '$parsed' => $parsed], true));

outputs:

array (
  '$buf' => ':server.name 372 BotNick :Who left a carriage return  in here?
:server.name 376 BotNick :End of message of the day.
:server.name 251 BotNick :There are',
  '$parsed' => 
  array (
    0 => 
    array (
      'prefix' => ':server.name',
      'servername' => 'server.name',
      'command' => '375',
      'params' => 
      array (
        1 => 'server.name message of the day',
        'all' => 'server.name message of the day',
      ),
      'message' => ':server.name 375 BotNick :server.name message of the day
',
      'code' => 'RPL_MOTDSTART',
      'target' => 'BotNick',
      'tail' => ':server.name 372 BotNick :Who left a carriage return  in here?
:server.name 376 BotNick :End of message of the day.
:server.name 251 BotNick :There are',
    ),
  ),
)

Calling consumeAll() again would return an empty array, and leave the buffer unchanged; it would never progress beyond the invalid line.

Perhaps parseAll() should have a way of failing gracefully if an invalid CRLF-terminated line is received.

[elazar]

@Renegade334 Does this change satisfy your use case?

[Renegade334]

Will have a look. As committed, the second group in $not_crlf is a capturing one, btw!

There are a couple of other ways that the parser could handle such lines: it could strip the invalid characters from the string entirely, or it could reject the entire line but carry on somehow. I am inclined towards stripping as the way to go, as they are invalid whitespace characters that shouldn't be there anyway.

The third invalid character is the NUL byte, which should probably be included for completeness.

[elazar]

Fixed the noted capturing group. The NUL byte is included in alternating groups in the patterns containing $not_crlf.

[Renegade334]

Sure, but that means that any NUL byte in the input will cause parse() to return null as was with isolated CR/LF characters.

[elazar]

Per RFC 2812 Section 2.3.1 Note 2:

The NUL (%x00) character is not special in message framing, and
basically could end up inside a parameter, but it would cause
extra complexities in normal C string handling. Therefore, NUL
is not allowed within messages.

Since this doesn't apply to PHP strings, we could allow NUL bytes and optionally strip them out after parsing. Thoughts?

[Renegade334]

Well, it's still directly forbidden by the RFC, so I don't think it needs to be optional.

Edit: How about ae752e2?

[elazar]

Stray NUL, CR, and LF bytes will now be stripped from messages prior to them being parsed. This should address the reported issue of such bytes causing the parser to hang.