protocolbuffers / protobuf

Protocol Buffers - Google's data interchange format
http://protobuf.dev
Other
65.68k stars 15.5k forks source link

Big numbers (uint64, fixed64) bug in PHP #9521

Closed zlodes closed 5 months ago

zlodes commented 2 years ago

Hi!

Generated setters for fixed64 or uint64 are using intval($var) in case of PHP_INT_SIZE == 8 (any 64x system).

But max value of uint64 is greater than PHP_INT_MAX: 2^64-1 vs. 2^63-1.

I have written tests: https://github.com/zlodes/protobuf-bugs-demo There are results: https://github.com/zlodes/protobuf-bugs-demo/runs/5235953996?check_suite_focus=true

Code fragment from tests:

$foo = new Foo();

/**
 * Max uint64 value: 2^64-1
 * @see https://en.wikipedia.org/wiki/Integer_(computer_science)#Long_integer
 */
$value = '18446744073709551615'; // max uint64 value: 2^64-1

$foo->setFixed64($value);

self::assertEquals(
    $value,
    $foo->getFixed64() // It returns 9223372036854775807 (PHP_INT_MAX)
);

... and from generated class:

/**
 * Generated from protobuf field <code>fixed64 fixed64 = 1;</code>
 * @return int|string
 */
public function getFixed64()
{
    return $this->fixed64;
}

/**
 * Generated from protobuf field <code>fixed64 fixed64 = 1;</code>
 * @param int|string $var
 * @return $this
 */
public function setFixed64($var)
{
    GPBUtil::checkUint64($var);
    $this->fixed64 = $var;

    return $this;
}

More you can find here: https://github.com/zlodes/protobuf-bugs-demo

Environment:

PHP:

❯ php -v
PHP 8.1.3 (cli) (built: Feb 16 2022 14:36:17) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.3, Copyright (c) Zend Technologies
    with Xdebug v3.1.3, Copyright (c) 2002-2022, by Derick Rethans
    with Zend OPcache v8.1.3, Copyright (c), by Zend Technologies

Protoc

❯ protoc --version
libprotoc 3.19.4

google/protobuf package

❯ composer info google/protobuf
name     : google/protobuf
descrip. : proto library for PHP
keywords : proto
versions : * v3.19.4
...
inkeliz commented 2 years ago

PHP doesn't support uint64:

PHP does not support unsigned ints. int size can be determined using the constant PHP_INT_SIZE, maximum value using the constant PHP_INT_MAX, and minimum value using the constant PHP_INT_MIN.

In that case, what is your suggestion to fix that? The only way to return '18446744073709551615' is using string, which will have a performance penalty and you should use bcmath library to manipulate it anyway. Returning strings and numbers, mixed, will break any ===.

It's possible a duplicate of https://github.com/protocolbuffers/protobuf/issues/5216

zlodes commented 2 years ago

@inkeliz

The only way to return '18446744073709551615' is using string

Yes. And it would be correct. And then the developer may use bcmath or another way to operate with big number.

Now it looks like that method will return int|string (original PHPDoc reference) and method was written to return int if the number fits to int and string otherwise. But there is a mistake and it doesn't work properly.

And now to fix this mistake I should to remove GPBUtil::checkUint64($var); from generated code to make it works.

esorot commented 2 years ago

This is a known issue that we are working to prioritize

WarriorXK commented 1 year ago

Is there an update on this issue? I am also running into it.

zlodes commented 1 year ago

Just opened PR to fix that. I've waited for more than a year and decided to fix it by myself 😄

prattcmp commented 10 months ago

Does setValueUnwrapped() resolve this ticket?

github-actions[bot] commented 6 months ago

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

github-actions[bot] commented 5 months ago

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.