drslump / Protobuf-PHP

PHP implementation of Google's Protocol Buffers with a protoc plugin compiler
http://drslump.github.com/Protobuf-PHP/
MIT License
461 stars 163 forks source link

Message::_set allows potentially unsafe usage #69

Open afk11 opened 6 years ago

afk11 commented 6 years ago

Today I bumped my environment to PHP7.2 and saw a new failure. It came about while serializing a structure with a repeated field, we'll call it SomeRepeatedValue - I had inadvertently used something like this:

$object->setSomeRepeatedValue($val)

Message::_has will call count if the field is repeated which is the cause of this error.

What seems to be happening is Message::$SomeRepeatedValue is not set as an array, because (1) the field isn't an extension, and (2) the $idx was null (the default value)

The difference between the two cases:

idx=null
$this->$SomeRepeatedValue = $val
and serialization fails

idx=0
$this->{$SomeRepeatedValue}[$idx] = $val
and serialization succeeds

Calling Message::_set() should check if the tag is a repeated field, and if so, should change the idx default from null to 0.

Please note, I'm not an expert on protobufs, and haven't dug into the spec in detail, but it seems _set/_has/_get should be consistent in how they handle repeated values, and would best be accomplished in _set.