From 6fff091c49359ffd8550fc8228c79740b504a4fe Mon Sep 17 00:00:00 2001 From: Paul Yang Date: Fri, 21 Apr 2017 15:00:00 -0700 Subject: Throw exception when parsing invalid data. (#3000) --- Makefile.am | 1 + php/ext/google/protobuf/encode_decode.c | 10 +- .../Protobuf/Internal/GPBDecodeException.php | 47 +++++ php/src/Google/Protobuf/Internal/InputStream.php | 17 +- php/src/Google/Protobuf/Internal/Message.php | 73 ++++---- php/tests/encode_decode_test.php | 198 +++++++++++++++++++++ 6 files changed, 297 insertions(+), 49 deletions(-) create mode 100644 php/src/Google/Protobuf/Internal/GPBDecodeException.php diff --git a/Makefile.am b/Makefile.am index d0f033af..3b57b585 100644 --- a/Makefile.am +++ b/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 \ diff --git a/php/ext/google/protobuf/encode_decode.c b/php/ext/google/protobuf/encode_decode.c index 06dc1195..28bf18f4 100644 --- a/php/ext/google/protobuf/encode_decode.c +++ b/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; diff --git a/php/src/Google/Protobuf/Internal/GPBDecodeException.php b/php/src/Google/Protobuf/Internal/GPBDecodeException.php new file mode 100644 index 00000000..402d542f --- /dev/null +++ b/php/src/Google/Protobuf/Internal/GPBDecodeException.php @@ -0,0 +1,47 @@ += 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; diff --git a/php/src/Google/Protobuf/Internal/Message.php b/php/src/Google/Protobuf/Internal/Message.php index 887c86ca..cd15e0f0 100644 --- a/php/src/Google/Protobuf/Internal/Message.php +++ b/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); } } diff --git a/php/tests/encode_decode_test.php b/php/tests/encode_decode_test.php index 94adf793..288df569 100644 --- a/php/tests/encode_decode_test.php +++ b/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() -- cgit v1.2.3