Extended key checking to match the language of the memcached spec (#167)

Updated to php7 branch by Aaron Stone <aaron@serendipity.cx>

Author: Samantha Quiñones

php_memcached.c

@@ -202,15 +202,28 @@ static inline php_memc_object_t *php_memc_fetch_object(zend_object *obj) {
 	}                                                                                 \
 	memc_user_data = (php_memc_user_data_t *) memcached_get_user_data(intern->memc);
 
-#define MEMC_CHECK_KEY(intern, key)                      \
-	if (UNEXPECTED(ZSTR_LEN(key) == 0 ||                 \
-		ZSTR_LEN(key) > MEMC_OBJECT_KEY_MAX_LENGTH ||    \
-		(memcached_behavior_get(intern->memc,            \
-				MEMCACHED_BEHAVIOR_BINARY_PROTOCOL) ?    \
-		strchr(ZSTR_VAL(key), '\n') :                    \
-		strchr(ZSTR_VAL(key), ' ')))) {                  \
-		intern->rescode = MEMCACHED_BAD_KEY_PROVIDED;    \
-		RETURN_FALSE;                                    \
+static
+zend_bool s_memc_valid_key_binary(const char *key)
+{
+	return strchr(key, '\n') == NULL;
+}
+
+static
+zend_bool s_memc_valid_key_ascii(const char *key)
+{
+	while (*key && !iscntrl(*key) && !isspace(*key)) ++key;
+	return *key == '\0';
+}
+
+#define MEMC_CHECK_KEY(intern, key)                                               \
+	if (UNEXPECTED(ZSTR_LEN(key) == 0 ||                                          \
+		ZSTR_LEN(key) > MEMC_OBJECT_KEY_MAX_LENGTH ||                             \
+		(memcached_behavior_get(intern->memc, MEMCACHED_BEHAVIOR_BINARY_PROTOCOL) \
+				? !s_memc_valid_key_binary(ZSTR_VAL(key))                         \
+				: !s_memc_valid_key_ascii(ZSTR_VAL(key))                          \
+		))) {                                                                     \
+		intern->rescode = MEMCACHED_BAD_KEY_PROVIDED;                             \
+		RETURN_FALSE;                                                             \
 	}
 
 #ifdef HAVE_MEMCACHED_PROTOCOL
@@ -307,18 +320,14 @@ static
 PHP_INI_MH(OnUpdateSessionPrefixString)
 {
 	if (new_value && ZSTR_LEN(new_value) > 0) {
-		char *ptr = ZSTR_VAL(new_value);
-
-		while (*ptr != '\0') {
-			if (isspace (*ptr++)) {
-				php_error_docref(NULL, E_WARNING, "memcached.sess_prefix cannot contain whitespace characters");
-				return FAILURE;
-			}
-		}
 		if (ZSTR_LEN(new_value) > MEMCACHED_MAX_KEY) {
 			php_error_docref(NULL, E_WARNING, "memcached.sess_prefix too long (max: %d)", MEMCACHED_MAX_KEY - 1);
 			return FAILURE;
 		}
+		if (!s_memc_valid_key_ascii(ZSTR_VAL(new_value))) {
+			php_error_docref(NULL, E_WARNING, "memcached.sess_prefix cannot contain whitespace or control characters");
+			return FAILURE;
+		}
 	}
 	return OnUpdateString(entry, new_value, mh_arg1, mh_arg2, mh_arg3, stage);
 }

tests/keys.phpt

@@ -28,6 +28,11 @@ var_dump ($binary->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
 var_dump ($ascii->set (''/*empty key*/, 'this is a test'));
 var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
 
+for ($i=0;$i<32;$i++) {
+	var_dump ($ascii->set ('asciikeywithnonprintablechar-' . chr($i) . '-here', 'this is a test'));
+	var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+}
+
 var_dump ($ascii->set (str_repeat ('1234567890', 512), 'this is a test'));
 var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
 
@@ -46,4 +51,68 @@ bool(false)
 bool(true)
 bool(false)
 bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
+bool(false)
+bool(true)
 OK

tests/session_badconf_emptyprefix.phpt

@@ -24,5 +24,5 @@ session_write_close();
 echo "OK";
 
 --EXPECTF--
-Warning: ini_set(): memcached.sess_prefix cannot contain whitespace characters in %s on line %d
+Warning: ini_set(): memcached.sess_prefix cannot contain whitespace or control characters in %s on line %d
 OK

tests/session_badconf_prefix.phpt

@@ -18,7 +18,7 @@ ini_set('memcached.sess_prefix', str_repeat('a', 512));
 echo "OK";
 
 --EXPECTF--
-Warning: ini_set(): memcached.sess_prefix cannot contain whitespace characters in %s on line %d
+Warning: ini_set(): memcached.sess_prefix cannot contain whitespace or control characters in %s on line %d
 
 Warning: ini_set(): memcached.sess_prefix too long (max: %d) in %s on line %d
 OK