From 6cc0af36d9acdf8237a2b293e35c1353da6bdb3b Mon Sep 17 00:00:00 2001 From: Teddy Grenman Date: Mon, 24 May 2010 00:32:27 +0300 Subject: [PATCH 1/3] Cleaner version fixing crash on stats without servers. --- php_memcached.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/php_memcached.c b/php_memcached.c index 23d1eaa7..1c8e44ee 100644 --- a/php_memcached.c +++ b/php_memcached.c @@ -1789,7 +1789,6 @@ PHP_METHOD(Memcached, getStats) memcached_return status; struct callbackContext context = {0}; memcached_server_function callbacks[1]; - zend_bool free_stats = 1; MEMC_METHOD_INIT_VARS; if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "") == FAILURE) { @@ -1798,20 +1797,21 @@ PHP_METHOD(Memcached, getStats) MEMC_METHOD_FETCH_OBJECT; - stats = memcached_stat(m_obj->memc, NULL, &status); - /* this is because memcached_stat returns non-null pointer on 0 servers - * which breaks memcached_stat_free, this is not particulary clean */ - if (errno != 0 || stats == NULL) { - free_stats = 0; - } else if (memcached_server_count(m_obj->memc) == 0) { - free_stats = 0; + if (memcached_server_count(m_obj->memc) == 0) { + array_init(return_value); + return; } + errno = 0; + stats = memcached_stat(m_obj->memc, NULL, &status); + php_memc_handle_error(i_obj, status TSRMLS_CC); + if (errno == ENOMEM || stats == NULL) { + RETURN_FALSE; + } + if (status != MEMCACHED_SUCCESS && status != MEMCACHED_SOME_ERRORS) { - if (free_stats) { - memcached_stat_free(NULL, stats); - } + memcached_stat_free(NULL, stats); RETURN_FALSE; } @@ -1823,9 +1823,7 @@ PHP_METHOD(Memcached, getStats) context.return_value = return_value; memcached_server_cursor(m_obj->memc, callbacks, &context, 1); - if (free_stats) { - memcached_stat_free(NULL, stats); - } + memcached_stat_free(NULL, stats); } /* }}} */ From d0250b186a11607fe335f2f8c59bfded5ad97d1a Mon Sep 17 00:00:00 2001 From: Teddy Grenman Date: Sat, 14 Aug 2010 21:50:26 +0300 Subject: [PATCH 2/3] Fix bad bitwise cast from uint64 CAS to double. --- php_memcached.c | 51 +++++++++++--------- tests/experimental/delete_bykey.phpt | 2 +- tests/experimental/get_bykey_cas.phpt | 6 +-- tests/experimental/getdelayed_bykey_cas.phpt | 10 ++-- 4 files changed, 36 insertions(+), 33 deletions(-) diff --git a/php_memcached.c b/php_memcached.c index 1c8e44ee..c842d9e7 100644 --- a/php_memcached.c +++ b/php_memcached.c @@ -148,24 +148,6 @@ typedef unsigned long int uint32_t; return; \ } -#ifndef DVAL_TO_LVAL -#ifdef _WIN64 -# define DVAL_TO_LVAL(d, l) \ - if ((d) > LONG_MAX) { \ - (l) = (long)(unsigned long)(__int64) (d); \ - } else { \ - (l) = (long) (d); \ - } -#else -# define DVAL_TO_LVAL(d, l) \ - if ((d) > LONG_MAX) { \ - (l) = (unsigned long) (d); \ - } else { \ - (l) = (long) (d); \ - } -#endif -#endif - #define RETURN_FROM_GET RETURN_NULL() /**************************************** @@ -304,6 +286,23 @@ static memcached_return php_memc_do_serverlist_callback(const memcached_st *ptr, static memcached_return php_memc_do_stats_callback(const memcached_st *ptr, memcached_server_instance_st instance, void *in_context); static memcached_return php_memc_do_version_callback(const memcached_st *ptr, memcached_server_instance_st instance, void *in_context); +/**************************************** + Utility functions +****************************************/ + +static inline double uint64_to_double(uint64_t i) { + union {double d; uint64_t i;} x; + + x.i = i; + return x.d; +} + +static inline uint64_t double_to_uint64(double d) { + union {double d; uint64_t i;} x; + + x.d = d; + return x.i; +} /**************************************** Method implementations @@ -514,7 +513,7 @@ static void php_memc_get_impl(INTERNAL_FUNCTION_PARAMETERS, zend_bool by_key) } zval_dtor(cas_token); - ZVAL_DOUBLE(cas_token, (double)cas); + ZVAL_DOUBLE(cas_token, uint64_to_double(cas)); memcached_result_free(&result); @@ -735,7 +734,8 @@ static void php_memc_getMulti_impl(INTERNAL_FUNCTION_PARAMETERS, zend_bool by_ke add_assoc_zval_ex(return_value, res_key, res_key_len+1, value); if (cas_tokens) { cas = memcached_result_cas(&result); - add_assoc_double_ex(cas_tokens, res_key, res_key_len+1, (double)cas); + add_assoc_double_ex(cas_tokens, res_key, res_key_len+1, + uint64_to_double(cas)); } } @@ -941,7 +941,8 @@ PHP_METHOD(Memcached, fetch) add_assoc_zval_ex(return_value, ZEND_STRS("value"), value); if (cas != 0) { /* XXX: also check against ULLONG_MAX or memc_behavior */ - add_assoc_double_ex(return_value, ZEND_STRS("cas"), (double)cas); + add_assoc_double_ex(return_value, ZEND_STRS("cas"), + uint64_to_double(cas)); } memcached_result_free(&result); @@ -997,7 +998,8 @@ PHP_METHOD(Memcached, fetchAll) add_assoc_zval_ex(entry, ZEND_STRS("value"), value); if (cas != 0) { /* XXX: also check against ULLONG_MAX or memc_behavior */ - add_assoc_double_ex(entry, ZEND_STRS("cas"), (double)cas); + add_assoc_double_ex(entry, ZEND_STRS("cas"), + uint64_to_double(cas)); } add_next_index_zval(return_value, entry); } @@ -1375,7 +1377,7 @@ static void php_memc_cas_impl(INTERNAL_FUNCTION_PARAMETERS, zend_bool by_key) RETURN_FALSE; } - DVAL_TO_LVAL(cas_d, cas); + cas = double_to_uint64(cas_d); if (m_obj->compression) { flags |= MEMC_VAL_COMPRESSED; @@ -2879,7 +2881,8 @@ static int php_memc_do_result_callback(zval *zmemc_obj, zend_fcall_info *fci, add_assoc_stringl_ex(z_result, ZEND_STRS("key"), res_key, res_key_len, 1); add_assoc_zval_ex(z_result, ZEND_STRS("value"), value); if (cas != 0) { - add_assoc_double_ex(z_result, ZEND_STRS("cas"), (double)cas); + add_assoc_double_ex(z_result, ZEND_STRS("cas"), + uint64_to_double(cas)); } if (zend_call_function(fci, fcc TSRMLS_CC) == FAILURE) { diff --git a/tests/experimental/delete_bykey.phpt b/tests/experimental/delete_bykey.phpt index 1face456..92afe9e4 100644 --- a/tests/experimental/delete_bykey.phpt +++ b/tests/experimental/delete_bykey.phpt @@ -37,4 +37,4 @@ A BAD KEY WAS PROVIDED/CHARACTERS OUT OF RANGE bool(false) NOT FOUND bool(false) -%rPROTOCOL ERROR|NOT FOUND|WRITE FAILURE%r +%rCLIENT ERROR|PROTOCOL ERROR|NOT FOUND|WRITE FAILURE%r diff --git a/tests/experimental/get_bykey_cas.phpt b/tests/experimental/get_bykey_cas.phpt index df94373c..8cc9e73c 100644 --- a/tests/experimental/get_bykey_cas.phpt +++ b/tests/experimental/get_bykey_cas.phpt @@ -50,13 +50,13 @@ var_dump($cas); var_dump($m->getByKey('foo', 'foo')); --EXPECTF-- int(1) -float(%d) +float(%f) SUCCESS int(1) -float(%d) +float(%f) SUCCESS string(4) "asdf" -float(%d) +float(%f) SUCCESS NULL NULL diff --git a/tests/experimental/getdelayed_bykey_cas.phpt b/tests/experimental/getdelayed_bykey_cas.phpt index b874e739..e954296e 100644 --- a/tests/experimental/getdelayed_bykey_cas.phpt +++ b/tests/experimental/getdelayed_bykey_cas.phpt @@ -39,7 +39,7 @@ array(3) { ["value"]=> string(8) "foo-data" ["cas"]=> - float(%d) + float(%f) } array(3) { ["key"]=> @@ -47,7 +47,7 @@ array(3) { ["value"]=> string(8) "bar-data" ["cas"]=> - float(%d) + float(%f) } array(3) { ["key"]=> @@ -55,7 +55,7 @@ array(3) { ["value"]=> string(8) "baz-data" ["cas"]=> - float(%d) + float(%f) } array(3) { ["key"]=> @@ -63,7 +63,7 @@ array(3) { ["value"]=> string(8) "lol-data" ["cas"]=> - float(%d) + float(%f) } array(3) { ["key"]=> @@ -71,5 +71,5 @@ array(3) { ["value"]=> string(8) "kek-data" ["cas"]=> - float(%d) + float(%f) } From 4462dacde095211b4e843aa83f3e563cbfa4c23d Mon Sep 17 00:00:00 2001 From: Teddy Grenman Date: Tue, 14 Sep 2010 23:34:31 +0300 Subject: [PATCH 3/3] Fix PECL Bug #18639, double free in getServerByKey. Bug report and fix, courtesy of Kevin Bowman. --- php_memcached.c | 7 ++++++- tests/bug_18639.phpt | 23 +++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 tests/bug_18639.phpt diff --git a/php_memcached.c b/php_memcached.c index c842d9e7..350e62ea 100644 --- a/php_memcached.c +++ b/php_memcached.c @@ -1779,7 +1779,12 @@ PHP_METHOD(Memcached, getServerByKey) add_assoc_string(return_value, "host", server->hostname, 1); add_assoc_long(return_value, "port", server->port); add_assoc_long(return_value, "weight", server->weight); - memcached_server_free(server); + + /* memcached_server_add(3) states that the server instance is cloned. */ + /* In actuality it is not, possibly a bug in libmemcached 0.40. */ + /* remove server freeing */ + + /* memcached_server_free(server); */ } /* }}} */ diff --git a/tests/bug_18639.phpt b/tests/bug_18639.phpt new file mode 100644 index 00000000..9deb7492 --- /dev/null +++ b/tests/bug_18639.phpt @@ -0,0 +1,23 @@ +--TEST-- +Memcached::getServerByKey(): Bug pecl#18639 (Segfault in getServerByKet) +--SKIPIF-- + +--FILE-- +addServer('127.0.0.1', 11211); +var_dump($m->set('test', 'test1')); +var_dump($m->getServerByKey('1')); + +--EXPECTF-- +bool(true) +array(3) { + ["host"]=> + string(9) "127.0.0.1" + ["port"]=> + int(11211) + ["weight"]=> + int(0) +}