Verify that the blocking pop timeout value is a non-negative integer

diff --git a/src/t_list.c b/src/t_list.c
index 2a98103333b71d9014983b78e0b05ea6fe8cccd9..43d292b6d6642ff7b40a40ac33f2c7191235716b 100644 (file)
--- a/src/t_list.c
+++ b/src/t_list.c
@@ -782,9 +782,20 @@ int handleClientsWaitingListPush(redisClient *c, robj *key, robj *ele) {
 /* Blocking RPOP/LPOP */
 void blockingPopGenericCommand(redisClient *c, int where) {
     robj *o;
 /* Blocking RPOP/LPOP */
 void blockingPopGenericCommand(redisClient *c, int where) {
     robj *o;

+    long long lltimeout;

     time_t timeout;
     int j;
 
     time_t timeout;
     int j;
 

+    /* Make sure timeout is an integer value */
+    if (getLongLongFromObjectOrReply(c,c->argv[c->argc-1],&lltimeout,
+            "timeout is not an integer") != REDIS_OK) return;
+
+    /* Make sure the timeout is not negative */
+    if (lltimeout < 0) {
+        addReplySds(c,sdsnew("-ERR timeout is negative\r\n"));
+        return;
+    }
+

     for (j = 1; j < c->argc-1; j++) {
         o = lookupKeyWrite(c->db,c->argv[j]);
         if (o != NULL) {
     for (j = 1; j < c->argc-1; j++) {
         o = lookupKeyWrite(c->db,c->argv[j]);
         if (o != NULL) {


@@ -823,8 +834,9 @@ void blockingPopGenericCommand(redisClient *c, int where) {
             }
         }
     }
             }
         }
     }

+

     /* If the list is empty or the key does not exists we must block */
     /* If the list is empty or the key does not exists we must block */

-    timeout = strtol(c->argv[c->argc-1]->ptr,NULL,10);
+    timeout = lltimeout;

     if (timeout > 0) timeout += time(NULL);
     blockForKeys(c,c->argv+1,c->argc-2,timeout);
 }
     if (timeout > 0) timeout += time(NULL);
     blockForKeys(c,c->argv+1,c->argc-2,timeout);
 }

diff --git a/tests/unit/type/list.tcl b/tests/unit/type/list.tcl
index d3ed90ecc0fc174534983fe60a66bd3260aded9c..1a85c809c7ae3e5e2ac6b3126d8ad4468a194cff 100644 (file)
--- a/tests/unit/type/list.tcl
+++ b/tests/unit/type/list.tcl
@@ -139,6 +139,28 @@ start_server {
             assert_equal 0 [r exists blist1]
         }
 
             assert_equal 0 [r exists blist1]
         }
 

+        test "$pop: with negative timeout" {
+            set rd [redis_deferring_client]
+            $rd $pop blist1 -1
+            assert_error "ERR*is negative*" {$rd read}
+        }
+
+        test "$pop: with non-integer timeout" {
+            set rd [redis_deferring_client]
+            $rd $pop blist1 1.1
+            assert_error "ERR*not an integer*" {$rd read}
+        }
+
+        test "$pop: with zero timeout should block indefinitely" {
+            # To test this, use a timeout of 0 and wait a second.
+            # The blocking pop should still be waiting for a push.
+            set rd [redis_deferring_client]
+            $rd $pop blist1 0
+            after 1000
+            r rpush blist1 foo
+            assert_equal {blist1 foo} [$rd read]
+        }
+

         test "$pop: second argument is not a list" {
             set rd [redis_deferring_client]
             r del blist1 blist2
         test "$pop: second argument is not a list" {
             set rd [redis_deferring_client]
             r del blist1 blist2