Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

increment can't bigger than 4294967295 #58

Closed
eibisee opened this issue Jan 15, 2013 · 7 comments
Closed

increment can't bigger than 4294967295 #58

eibisee opened this issue Jan 15, 2013 · 7 comments

Comments

@eibisee
Copy link

eibisee commented Jan 15, 2013

$o = new Memcached();
$o->addServers( array(
array('127.0.0.1', 11211, 100)
));
$o->set('ddd', 0, 0);

$x = $o->increment('ddd', 4294967295); //OK
$x = $o->increment('ddd', 4294967296); //Error.

system is 64Bit.

@mhagstrand
Copy link
Contributor

This is the result of an overflow. The value of 4294967296 (2^32) is just overflowing to 0. Incrementing by 4294967297 will result in an increment by 1.

PHP-memcached is casting the offset value of the increment and decrement function to an unsigned int. This can be seen in the php_memcached.c file with the code below:

if (incr) {
     status = memcached_increment(m_obj->memc, key, key_len, (unsigned int)offset, &value);
} else {

Fixing this is NOT as simple as just removing the cast from php_memcached.c. The memcached_increment function in libmemcached has a parameter called offset which is a uint32_t. This can be seen in the libmemcached file libmemcached/auto.cc with the code below:

memcached_return_t memcached_increment(memcached_st *memc,
                                       const char *key, size_t key_length,
                                       uint32_t offset,
                                       uint64_t *value)
{
      return memcached_increment_by_key(memc, key, key_length, key, key_length, offset, value);
} 

You should probably file this issue with libmemcached. The function memcached_increment_by_key uses a uint64_t for the offset. It could be used by PHP-memcached instead but the API docs for libmemcached still list the offset parameter in memcached_increment_by_key as uint32_t. That can be seen here:
http://docs.libmemcached.org/memcached_auto.html

@sodabrew
Copy link
Contributor

Confirmed this is still a bug in 3.0.0 with PHP 7.0 and PHP 7.1.

@sodabrew
Copy link
Contributor

Ah, the value can be 64-bit, but the increment is limited to 32-bits per API call. The protocol does allow 64-bit increments, so it might only be an API issue in libmemcached: https://github.com/memcached/memcached/blob/master/doc/protocol.txt#L274

@mhagstrand
Copy link
Contributor

mhagstrand commented Jan 25, 2017

The problem still exists in the libmemcached:
http://bazaar.launchpad.net/~tangent-trunk/libmemcached/1.2/view/head:/libmemcached/auto.cc#L149

It is possible to work around this problem by replacing memcached_increment with memcached_increment_by_key in the code below.
https://github.com/php-memcached-dev/php-memcached/blob/master/php_memcached.c#L2224

That would look something like this:

    if (by_key) {
      if (incr) {
        status = memcached_increment_by_key(intern->memc, ZSTR_VAL(server_key), ZSTR_LEN(server_key), ZSTR_VAL(key), ZSTR_LEN(key), offset, &value);
      } else {
        status = memcached_decrement_by_key(intern->memc, ZSTR_VAL(server_key), ZSTR_LEN(server_key), ZSTR_VAL(key), ZSTR_LEN(key), offset, &value);
      }
    } else {
      if (incr) {
        status = memcached_increment_by_key(intern->memc, ZSTR_VAL(key), ZSTR_LEN(key), ZSTR_VAL(key), ZSTR_LEN(key), offset, &value);
      } else {
        status = memcached_decrement_by_key(intern->memc, ZSTR_VAL(key), ZSTR_LEN(key), ZSTR_VAL(key), ZSTR_LEN(key), offset, &value);
      }
    }

@sodabrew
Copy link
Contributor

The documentation is mis-matched to the actual code! Yes, memcached_increment_by_key accepts a 64-bit offset. This should be straightforward to fix in the master branch.

@mhagstrand
Copy link
Contributor

I can put together a PR if you want or you can take care of it if you want.

@sodabrew
Copy link
Contributor

Thanks for the offer! I've got it going in #306

@sodabrew sodabrew modified the milestones: 3.0.0, 3.1.0 Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants