Bug fix: When encoding, negative int32 values should be padded to int64 (#2660)

in order to be wire compatible.

8 files changed, 51 insertions, 24 deletions

diff --git a/php/ext/google/protobuf/upb.c b/php/ext/google/protobuf/upb.c
index 98daafc0..a497c8e1 100644
--- a/php/ext/google/protobuf/upb.c
+++ b/php/ext/google/protobuf/upb.c
@@ -9728,7 +9728,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 f75e0861..67eb1bee 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 bf052c2f..de5ca978 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 f6be8500..43525c58 100644
--- a/php/src/Google/Protobuf/Internal/Message.php
+++ b/php/src/Google/Protobuf/Internal/Message.php
@@ -582,9 +582,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 587ac352..8c6d9b68 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 992f1631..dfe3b353 100644
--- a/php/tests/encode_decode_test.php
+++ b/php/tests/encode_decode_test.php
@@ -22,7 +22,8 @@ class EncodeDecodeTest extends TestBase
         $this->expectFields($from);
 
         $data = $from->encode();
-        $this->assertSame(TestUtil::getGoldenTestMessage(), $data);
+        $this->assertSame(bin2hex(TestUtil::getGoldenTestMessage()),
+                          bin2hex($data));
     }
 
     public function testDecode()
@@ -173,4 +174,25 @@ class EncodeDecodeTest extends TestBase
         $m = new TestMessage();
         $m->decode($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());
+    }
 }
diff --git a/php/tests/php_implementation_test.php b/php/tests/php_implementation_test.php
index ac7c13dc..46d3e5b6 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(447, $m->byteSize());
+        $this->assertSame(472, $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 7f2aae15..07df5502 100644
--- a/php/tests/test_util.php
+++ b/php/tests/test_util.php
@@ -235,7 +235,7 @@ class TestUtil
     public static function getGoldenTestMessage()
     {
         return hex2bin(
-            "08D6FFFFFF0F" .
+            "08D6FFFFFFFFFFFFFFFF01" .
             "10D5FFFFFFFFFFFFFFFF01" .
             "182A" .
             "202B" .
@@ -253,8 +253,8 @@ class TestUtil
             "800101" .
             "8A01020821" .
 
-            "F801D6FFFFFF0F" .
-            "F801CCFFFFFF0F" .
+            "F801D6FFFFFFFFFFFFFFFF01" .
+            "F801CCFFFFFFFFFFFFFFFF01" .
             "8002D5FFFFFFFFFFFFFFFF01" .
             "8002CBFFFFFFFFFFFFFFFF01" .
             "88022A" .
@@ -288,7 +288,7 @@ class TestUtil
             "FA02020822" .
             "FA02020823" .
 
-            "BA040C08C2FFFFFF0F10C2FFFFFF0F" .
+            "BA041608C2FFFFFFFFFFFFFFFF0110C2FFFFFFFFFFFFFFFF01" .
             "C2041608C1FFFFFFFFFFFFFFFF0110C1FFFFFFFFFFFFFFFF01" .
             "CA0404083E103E" .
             "D20404083F103F" .
@@ -401,7 +401,7 @@ class TestUtil
     public static function getGoldenTestPackedMessage()
     {
         return hex2bin(
-            "D2050AD6FFFFFF0FCCFFFFFF0F" .
+            "D20514D6FFFFFFFFFFFFFFFF01CCFFFFFFFFFFFFFFFF01" .
             "DA0514D5FFFFFFFFFFFFFFFF01CBFFFFFFFFFFFFFFFF01" .
             "E205022A34" .
             "EA05022B35" .
@@ -421,7 +421,7 @@ class TestUtil
     public static function getGoldenTestUnpackedMessage()
     {
         return hex2bin(
-            "D005D6FFFFFF0FD005CCFFFFFF0F" .
+            "D005D6FFFFFFFFFFFFFFFF01D005CCFFFFFFFFFFFFFFFF01" .
             "D805D5FFFFFFFFFFFFFFFF01D805CBFFFFFFFFFFFFFFFF01" .
             "E0052AE00534" .
             "E8052BE80535" .