From f23869c6154d8b083ee3417fac277bc25e13a4ac Mon Sep 17 00:00:00 2001 From: Paul Yang Date: Tue, 7 Feb 2017 21:33:28 -0800 Subject: [PATCH] Bug fix: When encoding, negative int32 values should be padded to int64 (#2660) in order to be wire compatible. --- php/ext/google/protobuf/upb.c | 2 +- php/src/Google/Protobuf/Internal/GPBWire.php | 8 +++++-- .../Google/Protobuf/Internal/InputStream.php | 1 - php/src/Google/Protobuf/Internal/Message.php | 4 +++- .../Google/Protobuf/Internal/OutputStream.php | 15 +++++-------- php/tests/encode_decode_test.php | 21 +++++++++++++++++++ php/tests/php_implementation_test.php | 9 ++++++-- php/tests/test_util.php | 12 +++++------ 8 files changed, 49 insertions(+), 23 deletions(-) diff --git a/php/ext/google/protobuf/upb.c b/php/ext/google/protobuf/upb.c index e0c56f8ed5..aca1eb6e42 100644 --- a/php/ext/google/protobuf/upb.c +++ b/php/ext/google/protobuf/upb.c @@ -10759,7 +10759,7 @@ static size_t encode_strbuf(void *c, const void *hd, const char *buf, T(double, double, dbl2uint64, encode_fixed64) T(float, float, flt2uint32, encode_fixed32) T(int64, int64_t, uint64_t, encode_varint) -T(int32, int32_t, uint32_t, encode_varint) +T(int32, int32_t, int64_t, encode_varint) T(fixed64, uint64_t, uint64_t, encode_fixed64) T(fixed32, uint32_t, uint32_t, encode_fixed32) T(bool, bool, bool, encode_varint) diff --git a/php/src/Google/Protobuf/Internal/GPBWire.php b/php/src/Google/Protobuf/Internal/GPBWire.php index f75e086132..67eb1bee65 100644 --- a/php/src/Google/Protobuf/Internal/GPBWire.php +++ b/php/src/Google/Protobuf/Internal/GPBWire.php @@ -403,10 +403,14 @@ class GPBWire return self::varint32Size($tag); } - public static function varint32Size($value) + public static function varint32Size($value, $sign_extended = false) { if ($value < 0) { - return 5; + if ($sign_extended) { + return 10; + } else { + return 5; + } } if ($value < (1 << 7)) { return 1; diff --git a/php/src/Google/Protobuf/Internal/InputStream.php b/php/src/Google/Protobuf/Internal/InputStream.php index bf052c2f82..de5ca97815 100644 --- a/php/src/Google/Protobuf/Internal/InputStream.php +++ b/php/src/Google/Protobuf/Internal/InputStream.php @@ -70,7 +70,6 @@ class InputStream private $total_bytes_read; const MAX_VARINT_BYTES = 10; - const MAX_VARINT32_BYTES = 5; const DEFAULT_RECURSION_LIMIT = 100; const DEFAULT_TOTAL_BYTES_LIMIT = 33554432; // 32 << 20, 32MB diff --git a/php/src/Google/Protobuf/Internal/Message.php b/php/src/Google/Protobuf/Internal/Message.php index ca4fde028b..887c86ca6e 100644 --- a/php/src/Google/Protobuf/Internal/Message.php +++ b/php/src/Google/Protobuf/Internal/Message.php @@ -772,9 +772,11 @@ class Message case GPBType::SFIXED64: $size += 8; break; - case GPBType::UINT32: case GPBType::INT32: case GPBType::ENUM: + $size += GPBWire::varint32Size($value, true); + break; + case GPBType::UINT32: $size += GPBWire::varint32Size($value); break; case GPBType::UINT64: diff --git a/php/src/Google/Protobuf/Internal/OutputStream.php b/php/src/Google/Protobuf/Internal/OutputStream.php index 587ac352d8..8c6d9b6807 100644 --- a/php/src/Google/Protobuf/Internal/OutputStream.php +++ b/php/src/Google/Protobuf/Internal/OutputStream.php @@ -39,7 +39,6 @@ class OutputStream private $buffer_size; private $current; - const MAX_VARINT32_BYTES = 5; const MAX_VARINT64_BYTES = 10; public function __construct($size) @@ -56,8 +55,8 @@ class OutputStream public function writeVarint32($value) { - $bytes = str_repeat(chr(0), self::MAX_VARINT32_BYTES); - $size = self::writeVarintToArray($value, $bytes, true); + $bytes = str_repeat(chr(0), self::MAX_VARINT64_BYTES); + $size = self::writeVarintToArray($value, $bytes); return $this->writeRaw($bytes, $size); } @@ -102,20 +101,16 @@ class OutputStream return true; } - private static function writeVarintToArray($value, &$buffer, $trim = false) + private static function writeVarintToArray($value, &$buffer) { $current = 0; $high = 0; $low = 0; if (PHP_INT_SIZE == 4) { - GPBUtil::divideInt64ToInt32($value, $high, $low, $trim); + GPBUtil::divideInt64ToInt32($value, $high, $low); } else { - if ($trim) { - $low = $value & 0xFFFFFFFF; - } else { - $low = $value; - } + $low = $value; } while ($low >= 0x80 || $low < 0) { diff --git a/php/tests/encode_decode_test.php b/php/tests/encode_decode_test.php index b54b12392e..ba4fff6971 100644 --- a/php/tests/encode_decode_test.php +++ b/php/tests/encode_decode_test.php @@ -190,6 +190,27 @@ class EncodeDecodeTest extends TestBase $m->mergeFromString($data); } + public function testEncodeNegativeInt32() + { + $m = new TestMessage(); + $m->setOptionalInt32(-1); + $data = $m->encode(); + $this->assertSame("08ffffffffffffffffff01", bin2hex($data)); + } + + public function testDecodeNegativeInt32() + { + $m = new TestMessage(); + $this->assertEquals(0, $m->getOptionalInt32()); + $m->decode(hex2bin("08ffffffffffffffffff01")); + $this->assertEquals(-1, $m->getOptionalInt32()); + + $m = new TestMessage(); + $this->assertEquals(0, $m->getOptionalInt32()); + $m->decode(hex2bin("08ffffffff0f")); + $this->assertEquals(-1, $m->getOptionalInt32()); + } + # TODO(teboring): Add test back when php implementation is ready for json # encode/decode. # public function testJsonEncode() diff --git a/php/tests/php_implementation_test.php b/php/tests/php_implementation_test.php index ec6b8d5a77..e124980820 100644 --- a/php/tests/php_implementation_test.php +++ b/php/tests/php_implementation_test.php @@ -469,6 +469,11 @@ class ImplementationTest extends TestBase $output = new OutputStream(3); $output->writeVarint32(16384); $this->assertSame(hex2bin('808001'), $output->getData()); + + // Negative numbers are padded to be compatible with int64. + $output = new OutputStream(10); + $output->writeVarint32(-43); + $this->assertSame(hex2bin('D5FFFFFFFFFFFFFFFF01'), $output->getData()); } public function testWriteVarint64() @@ -496,13 +501,13 @@ class ImplementationTest extends TestBase { $m = new TestMessage(); TestUtil::setTestMessage($m); - $this->assertSame(481, $m->byteSize()); + $this->assertSame(506, $m->byteSize()); } public function testPackedByteSize() { $m = new TestPackedMessage(); TestUtil::setTestPackedMessage($m); - $this->assertSame(156, $m->byteSize()); + $this->assertSame(166, $m->byteSize()); } } diff --git a/php/tests/test_util.php b/php/tests/test_util.php index 9c8e34d2ad..61f94aa108 100644 --- a/php/tests/test_util.php +++ b/php/tests/test_util.php @@ -321,7 +321,7 @@ class TestUtil public static function getGoldenTestMessage() { return hex2bin( - "08D6FFFFFF0F" . + "08D6FFFFFFFFFFFFFFFF01" . "10D5FFFFFFFFFFFFFFFF01" . "182A" . "202B" . @@ -339,8 +339,8 @@ class TestUtil "800101" . "8A01020821" . - "F801D6FFFFFF0F" . - "F801CCFFFFFF0F" . + "F801D6FFFFFFFFFFFFFFFF01" . + "F801CCFFFFFFFFFFFFFFFF01" . "8002D5FFFFFFFFFFFFFFFF01" . "8002CBFFFFFFFFFFFFFFFF01" . "88022A" . @@ -374,7 +374,7 @@ class TestUtil "FA02020822" . "FA02020823" . - "BA040C08C2FFFFFF0F10C2FFFFFF0F" . + "BA041608C2FFFFFFFFFFFFFFFF0110C2FFFFFFFFFFFFFFFF01" . "C2041608C1FFFFFFFFFFFFFFFF0110C1FFFFFFFFFFFFFFFF01" . "CA0404083E103E" . "D20404083F103F" . @@ -489,7 +489,7 @@ class TestUtil public static function getGoldenTestPackedMessage() { return hex2bin( - "D2050AD6FFFFFF0FCCFFFFFF0F" . + "D20514D6FFFFFFFFFFFFFFFF01CCFFFFFFFFFFFFFFFF01" . "DA0514D5FFFFFFFFFFFFFFFF01CBFFFFFFFFFFFFFFFF01" . "E205022A34" . "EA05022B35" . @@ -509,7 +509,7 @@ class TestUtil public static function getGoldenTestUnpackedMessage() { return hex2bin( - "D005D6FFFFFF0FD005CCFFFFFF0F" . + "D005D6FFFFFFFFFFFFFFFF01D005CCFFFFFFFFFFFFFFFF01" . "D805D5FFFFFFFFFFFFFFFF01D805CBFFFFFFFFFFFFFFFF01" . "E0052AE00534" . "E8052BE80535" .