You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/09/03 03:56:30 UTC
[GitHub] [incubator-kvrocks] git-hulk opened a new pull request, #808: Fix inline protocol don't allow LF only EOL
git-hulk opened a new pull request, #808:
URL: https://github.com/apache/incubator-kvrocks/pull/808
This closes #807
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun merged pull request #808: Fix inline protocol can't parse LF only EOL
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #808:
URL: https://github.com/apache/incubator-kvrocks/pull/808
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #808: Fix inline protocol can't parse LF only EOL
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #808:
URL: https://github.com/apache/incubator-kvrocks/pull/808#discussion_r962107065
##########
src/redis_request.cc:
##########
@@ -61,13 +71,13 @@ Status Request::Tokenize(evbuffer *input) {
} catch (std::exception &e) {
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (isOnlyLF || multi_bulk_len_ > (int64_t)PROTO_MULTI_MAX_SIZE) {
+ return Status(Status::NotOK, "Protocol error: invalid multibulk length");
+ }
Review Comment:
Why do we disallow `\n` as EOL in the multi-bulk scenario?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #808: Fix inline protocol can't parse LF only EOL
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #808:
URL: https://github.com/apache/incubator-kvrocks/pull/808#issuecomment-1236046749
> In the [RESP spec](https://redis.io/docs/reference/protocol-spec/) it seems only CRLF("\r\n") is the allowed delimiter. Does Redis implement a different manner actually?
Yes, RESP only allows CRLF but inline mode allows only LF end of line: https://github.com/redis/redis/blob/unstable/src/networking.c#L2080
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #808: Fix inline protocol can't parse LF only EOL
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #808:
URL: https://github.com/apache/incubator-kvrocks/pull/808#discussion_r962111519
##########
src/redis_request.cc:
##########
@@ -61,13 +71,13 @@ Status Request::Tokenize(evbuffer *input) {
} catch (std::exception &e) {
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (isOnlyLF || multi_bulk_len_ > (int64_t)PROTO_MULTI_MAX_SIZE) {
+ return Status(Status::NotOK, "Protocol error: invalid multibulk length");
+ }
Review Comment:
Redis also disallowed \n in multi bulk protocol, so we just keep consistent with Redis here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] tisonkun commented on a diff in pull request #808: Fix inline protocol can't parse LF only EOL
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #808:
URL: https://github.com/apache/incubator-kvrocks/pull/808#discussion_r962119531
##########
src/redis_request.cc:
##########
@@ -61,13 +71,13 @@ Status Request::Tokenize(evbuffer *input) {
} catch (std::exception &e) {
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (isOnlyLF || multi_bulk_len_ > (int64_t)PROTO_MULTI_MAX_SIZE) {
+ return Status(Status::NotOK, "Protocol error: invalid multibulk length");
+ }
Review Comment:
Thanks for your explanation. I get the point now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org