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/06/23 09:46:29 UTC
[GitHub] [incubator-kvrocks] willshS opened a new pull request, #652: Fix Wrongly parsed the RESP empty/null array (#633)
willshS opened a new pull request, #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652
**Now** : I only fix like:
`echo -e '*-1\r\n*2\r\n$3\r\nget\r\n$3\r\nkey\r\n' | nc 127.0.0.1 6666`
`echo -e '*2\r\n$3\r\nget\r\n$3\r\nkey\r\n*0\r\n*2\r\n$3\r\nget\r\n$3\r\nkey\r\n' | nc 127.0.0.1 6666`
Redis can have **"nil"** value. like:
kvrocks also behaves the same way.(I didn't modify anything)
--
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] PragmaTwice commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r904965510
##########
src/redis_request.cc:
##########
@@ -59,11 +59,15 @@ Status Request::Tokenize(evbuffer *input) {
svr_->stats_.IncrInbondBytes(len);
if (line[0] == '*') {
try {
- multi_bulk_len_ = std::stoull(std::string(line + 1, len-1));
+ multi_bulk_len_ = std::stoll(std::string(line + 1, len-1));
} catch (std::exception &e) {
free(line);
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (multi_bulk_len_ <= 0) {
Review Comment:
There is a condition `if( multi_bulk_len_ == 0)` in `src/redis_request.cc:111`
##########
src/redis_request.cc:
##########
@@ -59,11 +59,15 @@ Status Request::Tokenize(evbuffer *input) {
svr_->stats_.IncrInbondBytes(len);
if (line[0] == '*') {
try {
- multi_bulk_len_ = std::stoull(std::string(line + 1, len-1));
+ multi_bulk_len_ = std::stoll(std::string(line + 1, len-1));
} catch (std::exception &e) {
free(line);
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (multi_bulk_len_ <= 0) {
Review Comment:
There is a condition `if (multi_bulk_len_ == 0)` in `src/redis_request.cc:111`
--
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 #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905805893
##########
src/redis_request.cc:
##########
@@ -35,7 +35,7 @@
namespace Redis {
const size_t PROTO_INLINE_MAX_SIZE = 16 * 1024L;
const size_t PROTO_BULK_MAX_SIZE = 512 * 1024L * 1024L;
-const size_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
+const int64_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
Review Comment:
You may instead change `multi_bulk_len_ > PROTO_MULTI_MAX_SIZE` to `multi_bulk_len_ > (int64_t) PROTO_MULTI_MAX_SIZE`?
--
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 pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#issuecomment-1165210273
Shall this patch fix #633?
--
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 #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#issuecomment-1164300185
Cool, can you add some test cases in [protocol.tcl](https://github.com/apache/incubator-kvrocks/blob/unstable/tests/tcl/tests/unit/protocol.tcl) to make sure it has already fixed.
--
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] PragmaTwice commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905826626
##########
src/redis_request.cc:
##########
@@ -35,7 +35,7 @@
namespace Redis {
const size_t PROTO_INLINE_MAX_SIZE = 16 * 1024L;
const size_t PROTO_BULK_MAX_SIZE = 512 * 1024L * 1024L;
-const size_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
+const int64_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
Review Comment:
I also think it is better to keep `PROTO_MULTI_MAX_SIZE` typed `size_t`, since semantically it is a positive number.
--
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 #652: Fix Wrongly parsed the RESP empty/null array
Posted by GitBox <gi...@apache.org>.
tisonkun merged PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652
--
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] PragmaTwice commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r904963891
##########
src/redis_request.cc:
##########
@@ -59,11 +59,15 @@ Status Request::Tokenize(evbuffer *input) {
svr_->stats_.IncrInbondBytes(len);
if (line[0] == '*') {
try {
- multi_bulk_len_ = std::stoull(std::string(line + 1, len-1));
+ multi_bulk_len_ = std::stoll(std::string(line + 1, len-1));
} catch (std::exception &e) {
free(line);
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (multi_bulk_len_ <= 0) {
Review Comment:
Is an assignment `multi_bulk_len_ = 0` needed while the condition `multi_bulk_len_ < 0` holds?
--
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 #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#issuecomment-1165150022
> > Cool, can you add some test cases in [protocol.tcl](https://github.com/apache/incubator-kvrocks/blob/unstable/tests/tcl/tests/unit/protocol.tcl) to make sure it has already fixed?
>
> sure, that's what i should do.
Cool, thanks @willshS
--
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] PragmaTwice commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r904963891
##########
src/redis_request.cc:
##########
@@ -59,11 +59,15 @@ Status Request::Tokenize(evbuffer *input) {
svr_->stats_.IncrInbondBytes(len);
if (line[0] == '*') {
try {
- multi_bulk_len_ = std::stoull(std::string(line + 1, len-1));
+ multi_bulk_len_ = std::stoll(std::string(line + 1, len-1));
} catch (std::exception &e) {
free(line);
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (multi_bulk_len_ <= 0) {
Review Comment:
Is a `multi_bulk_len_ = 0` needed while `multi_bulk_len_ < 0` ?
--
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] willshS commented on pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
willshS commented on PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#issuecomment-1164518694
> Cool, can you add some test cases in [protocol.tcl](https://github.com/apache/incubator-kvrocks/blob/unstable/tests/tcl/tests/unit/protocol.tcl) to make sure it has already fixed?
sure, that's what i should do.
--
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] willshS commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
willshS commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905798847
##########
src/redis_request.cc:
##########
@@ -35,7 +35,7 @@
namespace Redis {
const size_t PROTO_INLINE_MAX_SIZE = 16 * 1024L;
const size_t PROTO_BULK_MAX_SIZE = 512 * 1024L * 1024L;
-const size_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
+const int64_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
Review Comment:
compiler warning. `size_t == int64_t`
--
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] PragmaTwice commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905826626
##########
src/redis_request.cc:
##########
@@ -35,7 +35,7 @@
namespace Redis {
const size_t PROTO_INLINE_MAX_SIZE = 16 * 1024L;
const size_t PROTO_BULK_MAX_SIZE = 512 * 1024L * 1024L;
-const size_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
+const int64_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
Review Comment:
I also think it is better to keep `PROTO_MULTI_MAX_SIZE` typed `size_t`.
--
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] PragmaTwice commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905826626
##########
src/redis_request.cc:
##########
@@ -35,7 +35,7 @@
namespace Redis {
const size_t PROTO_INLINE_MAX_SIZE = 16 * 1024L;
const size_t PROTO_BULK_MAX_SIZE = 512 * 1024L * 1024L;
-const size_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
+const int64_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
Review Comment:
I also think it is better to keep `PROTO_MULTI_MAX_SIZE` typed `size_t`, since semantically it is a positive contant.
##########
src/redis_request.cc:
##########
@@ -35,7 +35,7 @@
namespace Redis {
const size_t PROTO_INLINE_MAX_SIZE = 16 * 1024L;
const size_t PROTO_BULK_MAX_SIZE = 512 * 1024L * 1024L;
-const size_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
+const int64_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
Review Comment:
I also think it is better to keep `PROTO_MULTI_MAX_SIZE` typed `size_t`, since semantically it is a positive constant.
--
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] willshS commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
willshS commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905130235
##########
src/redis_request.cc:
##########
@@ -59,11 +59,15 @@ Status Request::Tokenize(evbuffer *input) {
svr_->stats_.IncrInbondBytes(len);
if (line[0] == '*') {
try {
- multi_bulk_len_ = std::stoull(std::string(line + 1, len-1));
+ multi_bulk_len_ = std::stoll(std::string(line + 1, len-1));
} catch (std::exception &e) {
free(line);
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (multi_bulk_len_ <= 0) {
Review Comment:
`state_ `is still `ArrayLen`, so it will read again and assign to `multi_bulk_len_`. it's a good habit that to add the assignment `multi_bulk_len_ = 0`. Thank you
--
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 #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905724277
##########
src/redis_request.cc:
##########
@@ -35,7 +35,7 @@
namespace Redis {
const size_t PROTO_INLINE_MAX_SIZE = 16 * 1024L;
const size_t PROTO_BULK_MAX_SIZE = 512 * 1024L * 1024L;
-const size_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
+const int64_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
Review Comment:
Why do you make this change?
--
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] willshS commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
willshS commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905130235
##########
src/redis_request.cc:
##########
@@ -59,11 +59,15 @@ Status Request::Tokenize(evbuffer *input) {
svr_->stats_.IncrInbondBytes(len);
if (line[0] == '*') {
try {
- multi_bulk_len_ = std::stoull(std::string(line + 1, len-1));
+ multi_bulk_len_ = std::stoll(std::string(line + 1, len-1));
} catch (std::exception &e) {
free(line);
return Status(Status::NotOK, "Protocol error: invalid multibulk length");
}
+ if (multi_bulk_len_ <= 0) {
Review Comment:
`state_ `is still `ArrayLen`, so it will read again and assign to `multi_bulk_len_`. it's a good habit that to add the assignment `multi_bulk_len_`. Thank you
--
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 #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#issuecomment-1165222890
> Shall this patch fix #633?
yes
--
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] willshS commented on a diff in pull request #652: Fix Wrongly parsed the RESP empty/null array (#633)
Posted by GitBox <gi...@apache.org>.
willshS commented on code in PR #652:
URL: https://github.com/apache/incubator-kvrocks/pull/652#discussion_r905875041
##########
src/redis_request.cc:
##########
@@ -35,7 +35,7 @@
namespace Redis {
const size_t PROTO_INLINE_MAX_SIZE = 16 * 1024L;
const size_t PROTO_BULK_MAX_SIZE = 512 * 1024L * 1024L;
-const size_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
+const int64_t PROTO_MULTI_MAX_SIZE = 1024 * 1024L;
Review Comment:
yes, you are right, it should be positive constant, thank you.
--
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