From 3ba2281f96098691d9d672ce8b484207379b09ee Mon Sep 17 00:00:00 2001
From: sundb <sundbcn@gmail.com>
Date: Wed, 2 Dec 2020 03:41:26 +0800
Subject: [PATCH] Improve dbid range check for SELECT, MOVE, COPY (#8085)
SELECT used to read the index into a `long` variable, and then pass it to a function
that takes an `int`, possibly causing an overflow before the range check.
Now all these commands use better and cleaner range check, and that also results in
a slight change of the error response in case of an invalid database index.
SELECT:
in the past it would have returned either `-ERR invalid DB index` (if not a number),
or `-ERR DB index is out of range` (if not between 1..16 or alike).
now it'll return either `-ERR value is out of range` (if not a number), or
`-ERR value is out of range, value must between -2147483648 and 2147483647`
(if not in the range for an int), or `-ERR DB index is out of range`
(if not between 0..16 or alike)
MOVE:
in the past it would only fail with `-ERR index out of range` no matter the reason.
now return the same errors as the new ones for SELECT mentioned above.
(i.e. unlike for SELECT even for a value like 17 we changed the error message)
COPY:
doesn't really matter how it behaved in the past (new command), new behavior is
like the above two.
---
src/db.c | 33 ++++++++++++++++-----------------
src/object.c | 10 ++++++++++
src/server.h | 1 +
tests/unit/keyspace.tcl | 4 ++--
4 files changed, 29 insertions(+), 19 deletions(-)
diff --git a/src/db.c b/src/db.c
index e51f79eaa..32baf0974 100644
--- a/src/db.c
+++ b/src/db.c
@@ -600,10 +600,9 @@ void existsCommand(client *c) {
}
void selectCommand(client *c) {
- long id;
+ int id;
- if (getLongFromObjectOrReply(c, c->argv[1], &id,
- "invalid DB index") != C_OK)
+ if (getIntFromObjectOrReply(c, c->argv[1], &id, NULL) != C_OK)
return;
if (server.cluster_enabled && id != 0) {
@@ -1022,8 +1021,8 @@ void renamenxCommand(client *c) {
void moveCommand(client *c) {
robj *o;
redisDb *src, *dst;
- int srcid;
- long long dbid, expire;
+ int srcid, dbid;
+ long long expire;
if (server.cluster_enabled) {
addReplyError(c,"MOVE is not allowed in cluster mode");
@@ -1034,11 +1033,11 @@ void moveCommand(client *c) {
src = c->db;
srcid = c->db->id;
- if (getLongLongFromObject(c->argv[2],&dbid) == C_ERR ||
- dbid < INT_MIN || dbid > INT_MAX ||
- selectDb(c,dbid) == C_ERR)
- {
- addReply(c,shared.outofrangeerr);
+ if (getIntFromObjectOrReply(c, c->argv[2], &dbid, NULL) != C_OK)
+ return;
+
+ if (selectDb(c,dbid) == C_ERR) {
+ addReplyError(c,"DB index is out of range");
return;
}
dst = c->db;
@@ -1084,8 +1083,8 @@ void moveCommand(client *c) {
void copyCommand(client *c) {
robj *o;
redisDb *src, *dst;
- int srcid;
- long long dbid, expire;
+ int srcid, dbid;
+ long long expire;
int j, replace = 0, delete = 0;
/* Obtain source and target DB pointers
@@ -1100,11 +1099,11 @@ void copyCommand(client *c) {
if (!strcasecmp(c->argv[j]->ptr,"replace")) {
replace = 1;
} else if (!strcasecmp(c->argv[j]->ptr, "db") && additional >= 1) {
- if (getLongLongFromObject(c->argv[j+1], &dbid) == C_ERR ||
- dbid < INT_MIN || dbid > INT_MAX ||
- selectDb(c, dbid) == C_ERR)
- {
- addReplyError(c,"invalid DB index");
+ if (getIntFromObjectOrReply(c, c->argv[j+1], &dbid, NULL) != C_OK)
+ return;
+
+ if (selectDb(c, dbid) == C_ERR) {
+ addReplyError(c,"DB index is out of range");
return;
}
dst = c->db;
diff --git a/src/object.c b/src/object.c
index d35ba0eaf..71eceb6d6 100644
--- a/src/object.c
+++ b/src/object.c
@@ -747,6 +747,16 @@ int getPositiveLongFromObjectOrReply(client *c, robj *o, long *target, const cha
return getRangeLongFromObjectOrReply(c, o, 0, LONG_MAX, target, msg);
}
+int getIntFromObjectOrReply(client *c, robj *o, int *target, const char *msg) {
+ long value;
+
+ if (getRangeLongFromObjectOrReply(c, o, INT_MIN, INT_MAX, &value, msg) != C_OK)
+ return C_ERR;
+
+ *target = value;
+ return C_OK;
+}
+
char *strEncoding(int encoding) {
switch(encoding) {
case OBJ_ENCODING_RAW: return "raw";
diff --git a/src/server.h b/src/server.h
index a46954096..0bcbeabfb 100644
--- a/src/server.h
+++ b/src/server.h
@@ -1872,6 +1872,7 @@ int getDoubleFromObject(const robj *o, double *target);
int getLongLongFromObject(robj *o, long long *target);
int getLongDoubleFromObject(robj *o, long double *target);
int getLongDoubleFromObjectOrReply(client *c, robj *o, long double *target, const char *msg);
+int getIntFromObjectOrReply(client *c, robj *o, int *target, const char *msg);
char *strEncoding(int encoding);
int compareStringObjects(robj *a, robj *b);
int collateStringObjects(robj *a, robj *b);
diff --git a/tests/unit/keyspace.tcl b/tests/unit/keyspace.tcl
index a74651db9..c69741814 100644
--- a/tests/unit/keyspace.tcl
+++ b/tests/unit/keyspace.tcl
@@ -217,7 +217,7 @@ start_server {tags {"keyspace"}} {
r set mykey foobar
catch {r copy mykey mynewkey DB notanumber} e
set e
- } {*ERR*invalid DB index}
+ } {ERR value is not an integer or out of range}
test {COPY can copy key expire metadata as well} {
r set mykey foobar ex 100
@@ -398,7 +398,7 @@ start_server {tags {"keyspace"}} {
r set mykey hello
catch {r move mykey notanumber} e
set e
- } {*ERR*index out of range}
+ } {ERR value is not an integer or out of range}
test {MOVE can move key expire metadata as well} {
r select 10