Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix behavior of Redis client #2874

Closed
wants to merge 1 commit into from
Closed

Fix behavior of Redis client #2874

wants to merge 1 commit into from

Conversation

atdt
Copy link
Contributor

@atdt atdt commented Jun 6, 2014

Because HHVM forces auto_detect_line_ending to be enabled (which is a separate
bug), if a CRLF is split across two TCP packets, fgets() is liable to get
confused and assume that the stream uses CR line ending. As a result, the
return value of Redis::sockReadLine() is liable to have either a trailing CR or
a leading LF, both of which are corruptions. This causes the client to throw an
exception, complaining of a protocol error.

The behavior of fgets() is documented here:
http://www.php.net/manual/en/function.fgets.php#101963

The Redis protocol spec affirms that responses terminate with CRLF and are
guaranteed not to contain either CR or LF in the response body:
http://redis.io/topics/protocol

Because HHVM forces auto_detect_line_ending to be enabled (which is a separate
bug), if a CRLF is split across two TCP packets, fgets() is liable to get
confused and assume that the stream uses CR line ending. As a result, the
return value of Redis::sockReadLine() is liable to have either a trailing CR or
a leading LF, both of which are corruptions. This causes the client to throw an
exception, complaining of a protocol error.

The behavior of fgets() is documented here:
http://www.php.net/manual/en/function.fgets.php#101963

The Redis protocol spec affirms that responses terminate with CRLF and are
guaranteed not to contain either CR or LF in the response body:
http://redis.io/topics/protocol
@JoelMarcey
Copy link
Contributor

@atdt Hi. I have pulled this in for review.

Internal Diff Number: D1370933

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants