From e91eae7be3d6076f5a66f4ef3b588d61df8ec1a2 Mon Sep 17 00:00:00 2001
From: Aaron Stone <aaron@serendipity.cx>
Date: Mon, 10 Jul 2017 14:46:20 -0700
Subject: [PATCH 1/2] Split tests for invalid keys ascii and binary

Test get command for invalid keys as well as set command
---
 package.xml            |   3 +-
 tests/keys.phpt        | 118 -------------------------
 tests/keys_ascii.phpt  | 190 +++++++++++++++++++++++++++++++++++++++++
 tests/keys_binary.phpt | 190 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 382 insertions(+), 119 deletions(-)
 delete mode 100644 tests/keys.phpt
 create mode 100644 tests/keys_ascii.phpt
 create mode 100644 tests/keys_binary.phpt

diff --git a/package.xml b/package.xml
index 2306344d..2cc8995b 100644
--- a/package.xml
+++ b/package.xml
@@ -147,7 +147,8 @@ Fixes
     <file role='test' name='incrdecr_invalid_key.phpt'/>
     <file role='test' name='incrdecr_bykey.phpt'/>
     <file role='test' name='invalid_options.phpt'/>
-    <file role='test' name='keys.phpt'/>
+    <file role='test' name='keys_ascii.phpt'/>
+    <file role='test' name='keys_binary.phpt'/>
     <file role='test' name='testdata.res'/>
     <file role='test' name='config.inc'/>
     <file role='test' name='sasl_basic.phpt'/>
diff --git a/tests/keys.phpt b/tests/keys.phpt
deleted file mode 100644
index f320e03f..00000000
--- a/tests/keys.phpt
+++ /dev/null
@@ -1,118 +0,0 @@
---TEST--
-Test different kind of keys
---SKIPIF--
-<?php include "skipif.inc";?>
---FILE--
-<?php
-
-include dirname (__FILE__) . '/config.inc';
-$binary = memc_get_instance (array (
-								Memcached::OPT_BINARY_PROTOCOL => true,
-							));
-
-$ascii = memc_get_instance ();
-$ascii->setOption(Memcached::OPT_VERIFY_KEY, 1);
-
-var_dump ($binary->set ('binary key with spaces', 'this is a test'));
-var_dump ($binary->getResultCode () == Memcached::RES_SUCCESS);
-
-var_dump ($binary->set ('binarykeywithnewline' . PHP_EOL, 'this is a test'));
-var_dump ($binary->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
-
-var_dump ($ascii->set ('ascii key with spaces', 'this is a test'));
-var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
-
-var_dump ($binary->set ('asciikeywithnewline' . PHP_EOL, 'this is a test'));
-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);
-
-echo "OK" . PHP_EOL;
-
---EXPECT--
-bool(true)
-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)
-bool(false)
-bool(true)
-bool(false)
-bool(true)
-bool(false)
-bool(true)
-bool(false)
-bool(true)
-OK
diff --git a/tests/keys_ascii.phpt b/tests/keys_ascii.phpt
new file mode 100644
index 00000000..f7e98894
--- /dev/null
+++ b/tests/keys_ascii.phpt
@@ -0,0 +1,190 @@
+--TEST--
+Test valid and invalid keys - ascii
+--SKIPIF--
+<?php include "skipif.inc";?>
+--FILE--
+<?php
+
+include dirname (__FILE__) . '/config.inc';
+$ascii = memc_get_instance (array (
+				Memcached::OPT_BINARY_PROTOCOL => false,
+				Memcached::OPT_VERIFY_KEY => false
+		));
+// libmemcached can verify keys, but these are tests are for our own
+// function s_memc_valid_key_ascii, so explicitly disable the checks
+// that libmemcached can perform.
+
+echo 'ASCII: SPACES' . PHP_EOL;
+var_dump ($ascii->set ('ascii key with spaces', 'this is a test'));
+var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+
+echo 'ASCII: NEWLINE' . PHP_EOL;
+var_dump ($ascii->set ('asciikeywithnewline' . PHP_EOL, 'this is a test'));
+var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+
+echo 'ASCII: EMPTY' . PHP_EOL;
+var_dump ($ascii->set (''/*empty key*/, 'this is a test'));
+var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+
+echo 'ASCII: TOO LONG' . PHP_EOL;
+var_dump ($ascii->set (str_repeat ('1234567890', 512), 'this is a test'));
+var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+
+echo 'ASCII: GET' . PHP_EOL;
+for ($i=0;$i<32;$i++) {
+	var_dump ($ascii->get ('asciikeywithnonprintablechar-' . chr($i) . '-here'));
+	var_dump ($ascii->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+}
+
+echo 'ASCII: SET' . PHP_EOL;
+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);
+}
+
+echo 'OK' . PHP_EOL;
+
+--EXPECT--
+ASCII: SPACES
+bool(false)
+bool(true)
+ASCII: NEWLINE
+bool(false)
+bool(true)
+ASCII: EMPTY
+bool(false)
+bool(true)
+ASCII: TOO LONG
+bool(false)
+bool(true)
+ASCII: GET
+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)
+ASCII: SET
+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
diff --git a/tests/keys_binary.phpt b/tests/keys_binary.phpt
new file mode 100644
index 00000000..f0ae67bd
--- /dev/null
+++ b/tests/keys_binary.phpt
@@ -0,0 +1,190 @@
+--TEST--
+Test valid and invalid keys - binary
+--SKIPIF--
+<?php include "skipif.inc";?>
+--FILE--
+<?php
+
+include dirname (__FILE__) . '/config.inc';
+$binary = memc_get_instance (array (
+		Memcached::OPT_BINARY_PROTOCOL => true,
+	));
+
+echo 'BINARY: SPACES' . PHP_EOL;
+var_dump ($binary->set ('binary key with spaces', 'this is a test'));
+var_dump ($binary->getResultCode () == Memcached::RES_SUCCESS);
+
+echo 'BINARY: NEWLINE' . PHP_EOL;
+var_dump ($binary->set ('binarykeywithnewline' . PHP_EOL, 'this is a test'));
+var_dump ($binary->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+
+echo 'BINARY: EMPTY' . PHP_EOL;
+var_dump ($binary->set (''/*empty key*/, 'this is a test'));
+var_dump ($binary->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+
+echo 'BINARY: TOO LONG' . PHP_EOL;
+var_dump ($binary->set (str_repeat ('1234567890', 512), 'this is a test'));
+var_dump ($binary->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+
+echo 'BINARY: GET' . PHP_EOL;
+// Only newline fails in binary mode (char 10)
+for ($i=0;$i<32;$i++) {
+	$binary->delete ('binarykeywithnonprintablechar-' . chr($i) . '-here');
+	var_dump ($binary->get ('binarykeywithnonprintablechar-' . chr($i) . '-here'));
+	var_dump ($binary->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+}
+
+echo 'BINARY: SET' . PHP_EOL;
+// Only newline fails in binary mode (char 10)
+for ($i=0;$i<32;$i++) {
+	var_dump ($binary->set ('binarykeywithnonprintablechar-' . chr($i) . '-here', 'this is a test'));
+	var_dump ($binary->getResultCode () == Memcached::RES_BAD_KEY_PROVIDED);
+	$binary->delete ('binarykeywithnonprintablechar-' . chr($i) . '-here');
+}
+
+echo 'OK' . PHP_EOL;
+
+--EXPECT--
+BINARY: SPACES
+bool(true)
+bool(true)
+BINARY: NEWLINE
+bool(false)
+bool(true)
+BINARY: EMPTY
+bool(false)
+bool(true)
+BINARY: TOO LONG
+bool(false)
+bool(true)
+BINARY: GET
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(true)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+bool(false)
+BINARY: SET
+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(false)
+bool(true)
+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)
+OK

From be93a459eedc54fbbf2dfa63babea38b730fc313 Mon Sep 17 00:00:00 2001
From: Aaron Stone <aaron@serendipity.cx>
Date: Mon, 10 Jul 2017 11:04:59 -0700
Subject: [PATCH 2/2] Fix the key validity tests to handle strings with
 embedded nulls

---
 php_memcached.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/php_memcached.c b/php_memcached.c
index 4449200b..65d11eb2 100644
--- a/php_memcached.c
+++ b/php_memcached.c
@@ -198,24 +198,30 @@ static inline php_memc_object_t *php_memc_fetch_object(zend_object *obj) {
 	(void)memc_user_data; /* avoid unused variable warning */
 
 static
-zend_bool s_memc_valid_key_binary(const char *key)
+zend_bool s_memc_valid_key_binary(zend_string *key)
 {
-	return strchr(key, '\n') == NULL;
+	return memchr(ZSTR_VAL(key), '\n', ZSTR_LEN(key)) == NULL;
 }
 
 static
-zend_bool s_memc_valid_key_ascii(const char *key)
+zend_bool s_memc_valid_key_ascii(zend_string *key)
 {
-	while (*key && !iscntrl(*key) && !isspace(*key)) ++key;
-	return *key == '\0';
+	const char *str = ZSTR_VAL(key);
+	size_t i, len = ZSTR_LEN(key);
+
+	for (i = 0; i < len; i++) {
+		if (iscntrl(str[i]) || isspace(str[i]))
+			return 0;
+	}
+	return 1;
 }
 
 #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))                          \
+				? !s_memc_valid_key_binary(key)                                   \
+				: !s_memc_valid_key_ascii(key)                                    \
 		))) {                                                                     \
 		intern->rescode = MEMCACHED_BAD_KEY_PROVIDED;                             \
 		RETURN_FALSE;                                                             \
@@ -309,7 +315,7 @@ PHP_INI_MH(OnUpdateSessionPrefixString)
 			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))) {
+		if (!s_memc_valid_key_ascii(new_value)) {
 			php_error_docref(NULL, E_WARNING, "memcached.sess_prefix cannot contain whitespace or control characters");
 			return FAILURE;
 		}