Throw exception when parsing invalid data. (#3000)

Makefile.am

@@ -646,6 +646,7 @@ php_EXTRA_DIST=                                                       \
   php/src/Google/Protobuf/Internal/EnumBuilderContext.php             \
   php/src/Google/Protobuf/Internal/GPBUtil.php                        \
   php/src/Google/Protobuf/Internal/FieldOptions_CType.php             \
+  php/src/Google/Protobuf/Internal/GPBDecodeException.php             \
   php/src/Google/Protobuf/descriptor.php                              \
   php/src/GPBMetadata/Google/Protobuf/Internal/Descriptor.php         \
   php/tests/array_test.php                                            \

php/ext/google/protobuf/encode_decode.c

@@ -261,13 +261,6 @@ static void* appendbytes_handler(void *closure,
 #endif
 }
 
-  static bool int32_handler(void* closure, const void* hd, 
-                                     int32_t val) {           
-    MessageHeader* msg = (MessageHeader*)closure;           
-    const size_t *ofs = hd;                                 
-    DEREF(message_data(msg), *ofs, int32_t) = val;            
-    return true;                                            
-  }
 // Handlers that append primitive values to a repeated field.
 #define DEFINE_SINGULAR_HANDLER(type, ctype)                \
   static bool type##_handler(void* closure, const void* hd, \
@@ -279,7 +272,7 @@ static void* appendbytes_handler(void *closure,
   }
 
 DEFINE_SINGULAR_HANDLER(bool,   bool)
-// DEFINE_SINGULAR_HANDLER(int32,  int32_t)
+DEFINE_SINGULAR_HANDLER(int32,  int32_t)
 DEFINE_SINGULAR_HANDLER(uint32, uint32_t)
 DEFINE_SINGULAR_HANDLER(float,  float)
 DEFINE_SINGULAR_HANDLER(int64,  int64_t)
@@ -1435,6 +1428,7 @@ PHP_METHOD(Message, mergeFromString) {
 
   char *data = NULL;
   PHP_PROTO_SIZE data_len;
+
   if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "s", &data, &data_len) ==
       FAILURE) {
     return;

php/src/Google/Protobuf/Internal/GPBDecodeException.php

@@ -0,0 +1,47 @@
+<?php
+
+// Protocol Buffers - Google's data interchange format
+// Copyright 2008 Google Inc.  All rights reserved.
+// https://developers.google.com/protocol-buffers/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+namespace Google\Protobuf\Internal;
+
+class GPBDecodeException extends \Exception
+{
+    public function __construct(
+        $message,
+        $code = 0,
+        \Exception $previous = null)
+    {
+        parent::__construct(
+            "Error occurred during parsing: " . $message,
+            $code,
+            $previous);
+    }
+}

php/src/Google/Protobuf/Internal/InputStream.php

@@ -330,6 +330,7 @@ class InputStream
      * passed unchanged to the corresponding call to popLimit().
      *
      * @param integer $byte_limit
+     * @throws Exception Fail to push limit.
      */
     public function pushLimit($byte_limit)
     {
@@ -339,19 +340,15 @@ class InputStream
 
         // security: byte_limit is possibly evil, so check for negative values
         // and overflow.
-        if ($byte_limit >= 0 && $byte_limit <= PHP_INT_MAX - $current_position) {
+        if ($byte_limit >= 0 &&
+            $byte_limit <= PHP_INT_MAX - $current_position &&
+            $byte_limit <= $this->current_limit - $current_position) {
             $this->current_limit = $current_position + $byte_limit;
+            $this->recomputeBufferLimits();
         } else {
-            // Negative or overflow.
-            $this->current_limit = PHP_INT_MAX;
+            throw new GPBDecodeException("Fail to push limit.");
         }
 
-        // We need to enforce all limits, not just the new one, so if the previous
-        // limit was before the new requested limit, we continue to enforce the
-        // previous limit.
-        $this->current_limit = min($this->current_limit, $old_limit);
-
-        $this->recomputeBufferLimits();
         return $old_limit;
     }
 
@@ -370,7 +367,7 @@ class InputStream
     }
 
     public function incrementRecursionDepthAndPushLimit(
-    $byte_limit, &$old_limit, &$recursion_budget)
+        $byte_limit, &$old_limit, &$recursion_budget)
     {
         $old_limit = $this->pushLimit($byte_limit);
         $recursion_limit = --$this->recursion_limit;

php/src/Google/Protobuf/Internal/Message.php

@@ -224,48 +224,57 @@ class Message
         switch ($field->getType()) {
             case GPBType::DOUBLE:
                 if (!GPBWire::readDouble($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside double field.");
                 }
                 break;
             case GPBType::FLOAT:
                 if (!GPBWire::readFloat($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside float field.");
                 }
                 break;
             case GPBType::INT64:
                 if (!GPBWire::readInt64($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside int64 field.");
                 }
                 break;
             case GPBType::UINT64:
                 if (!GPBWire::readUint64($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside uint64 field.");
                 }
                 break;
             case GPBType::INT32:
                 if (!GPBWire::readInt32($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside int32 field.");
                 }
                 break;
             case GPBType::FIXED64:
                 if (!GPBWire::readFixed64($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside fixed64 field.");
                 }
                 break;
             case GPBType::FIXED32:
                 if (!GPBWire::readFixed32($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside fixed32 field.");
                 }
                 break;
             case GPBType::BOOL:
                 if (!GPBWire::readBool($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside bool field.");
                 }
                 break;
             case GPBType::STRING:
                 // TODO(teboring): Add utf-8 check.
                 if (!GPBWire::readString($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside string field.");
                 }
                 break;
             case GPBType::GROUP:
@@ -280,43 +289,51 @@ class Message
                     $value = new $klass;
                 }
                 if (!GPBWire::readMessage($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside message.");
                 }
                 break;
             case GPBType::BYTES:
                 if (!GPBWire::readString($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside bytes field.");
                 }
                 break;
             case GPBType::UINT32:
                 if (!GPBWire::readUint32($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside uint32 field.");
                 }
                 break;
             case GPBType::ENUM:
                 // TODO(teboring): Check unknown enum value.
                 if (!GPBWire::readInt32($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside enum field.");
                 }
                 break;
             case GPBType::SFIXED32:
                 if (!GPBWire::readSfixed32($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside sfixed32 field.");
                 }
                 break;
             case GPBType::SFIXED64:
                 if (!GPBWire::readSfixed64($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside sfixed64 field.");
                 }
                 break;
             case GPBType::SINT32:
                 if (!GPBWire::readSint32($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside sint32 field.");
                 }
                 break;
             case GPBType::SINT64:
                 if (!GPBWire::readSint64($input, $value)) {
-                    return false;
+                    throw new GPBDecodeException(
+                        "Unexpected EOF inside sint64 field.");
                 }
                 break;
             default:
@@ -345,24 +362,21 @@ class Message
         }
 
         if ($value_format === GPBWire::NORMAL_FORMAT) {
-            if (!self::parseFieldFromStreamNoTag($input, $field, $value)) {
-                return false;
-            }
+            self::parseFieldFromStreamNoTag($input, $field, $value);
         } elseif ($value_format === GPBWire::PACKED_FORMAT) {
             $length = 0;
             if (!GPBWire::readInt32($input, $length)) {
-                return false;
+                throw new GPBDecodeException(
+                    "Unexpected EOF inside packed length.");
             }
             $limit = $input->pushLimit($length);
             $getter = $field->getGetter();
             while ($input->bytesUntilLimit() > 0) {
-                if (!self::parseFieldFromStreamNoTag($input, $field, $value)) {
-                    return false;
-                }
+                self::parseFieldFromStreamNoTag($input, $field, $value);
                 $this->$getter()[] = $value;
             }
             $input->popLimit($limit);
-            return true;
+            return;
         } else {
             return false;
         }
@@ -377,8 +391,6 @@ class Message
             $setter = $field->getSetter();
             $this->$setter($value);
         }
-
-        return true;
     }
 
     /**
@@ -567,7 +579,8 @@ class Message
      * specified message.
      *
      * @param string $data Binary protobuf data.
-     * @return bool Return true on success.
+     * @return null.
+     * @throws Exception Invalid data.
      */
     public function mergeFromString($data)
     {
@@ -595,9 +608,7 @@ class Message
               continue;
             }
 
-            if (!$this->parseFieldFromStream($tag, $input, $field)) {
-                return false;
-            }
+            $this->parseFieldFromStream($tag, $input, $field);
         }
     }
 

php/tests/encode_decode_test.php

@@ -211,6 +211,204 @@ class EncodeDecodeTest extends TestBase
         $this->assertEquals(-1, $m->getOptionalInt32());
     }
 
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidInt32()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('08'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidSubMessage()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('9A010108'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidInt64()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('10'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidUInt32()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('18'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidUInt64()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('20'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidSInt32()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('28'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidSInt64()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('30'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidFixed32()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('3D'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidFixed64()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('41'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidSFixed32()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('4D'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidSFixed64()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('51'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidFloat()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('5D'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidDouble()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('61'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidBool()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('68'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidStringLengthMiss()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('72'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidStringDataMiss()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('7201'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidBytesLengthMiss()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('7A'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidBytesDataMiss()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('7A01'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidEnum()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('8001'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidMessageLengthMiss()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('8A01'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidMessageDataMiss()
+    {
+        $m = new TestMessage();
+        $m->mergeFromString(hex2bin('8A0101'));
+    }
+
+    /**
+     * @expectedException Exception
+     */
+    public function testDecodeInvalidPackedMessageLength()
+    {
+        $m = new TestPackedMessage();
+        $m->mergeFromString(hex2bin('D205'));
+    }
+
     # TODO(teboring): Add test back when php implementation is ready for json
     # encode/decode.
     # public function testJsonEncode()