mergeFromJsonString dumps core for a valid RFC3339 timestamp

[vishal-wadhwa]

What version of protobuf and what language are you using?
Version: 3.10
Language: PHP

What operating system (Linux, Windows, ...) and version?
Linux

What runtime / compiler are you using (e.g., python version or gcc version)
php 7.2.24

What did you do?
Steps to reproduce the behavior:

1. Perform mergeFromJsonString on an RFC3339 compliant timestamp with a non zero minute time offset "2020-11-15T11:10:00+02:30". (Note: this works perfectly for "2020-11-15T11:10:00+02:00")

What did you expect to see
The json read correctly into \Google\Protobuf\Timestamp; as protobuf says that Timestamp (as a WKT) uses RFC3339.

What did you see instead?
Segmentation fault (core dumped)

Make sure you include information that can help us debug (full error message, exception listing, stack trace, logs).

Anything else we should know about your project / environment

If I followed along the code correctly, here the minutes are hardcoded as "00" effectively disallowing any timestamp with non-"00" offset in minutes.
https://github.com/protocolbuffers/protobuf/blob/master/php/ext/google/protobuf/php-upb.c#L6931

If I understood RFC3339 correctly, there should be no such restriction.

I could not find any existing open or closed issue which addressed this under php label, hence, I created one.

[haberman]

Can you try with the newest version of protobuf? The protobuf-PHP library has changed a lot since version 3.10.

[vishal-wadhwa]

@haberman issue is reproducible with protobuf 3.14 and php 7.4. Apparently, there is no seg fault which I observed for protobuf 3.10 and php 7.2 as in the issue description.

// test.proto
syntax = "proto3";

package test;

import "google/protobuf/timestamp.proto";
message TestMessage {
  google.protobuf.Timestamp time = 1;
}

// test.php
<?php

require __DIR__.'/build/gen/GPBMetadata/Test.php';
require __DIR__.'/build/gen/Test/TestMessage.php';

echo phpversion("protobuf").PHP_EOL;

$ok_time = '{"time":"2020-11-15T11:10:00+02:00"}';
$tm = new Test\TestMessage();
$tm->mergeFromJsonString($ok_time);
echo $tm->serializeToJsonString(true).PHP_EOL;

$not_ok_time = '{"time":"2020-11-15T11:10:00+02:30"}';
$tm2 = new Test\TestMessage();
$tm2->mergeFromJsonString($not_ok_time);
echo $tm2->serializeToJsonString(true).PHP_EOL;

Output

3.14.0
{"time":"2020-11-15T09:10:00Z"}
PHP Fatal error:  Uncaught Exception: Error occurred during parsing: Error parsing JSON @1:35: Malformed timestamp in .../proto-bug-repro/test.php:18
Stack trace:
#0 .../proto-bug-repro/test.php(18): Google\Protobuf\Internal\Message->mergeFromJsonString('{"time":"2020-1...')
#1 {main}
  thrown in .../proto-bug-repro/test.php on line 18

Fatal error: Uncaught Exception: Error occurred during parsing: Error parsing JSON @1:35: Malformed timestamp in .../proto-bug-repro/test.php:18
Stack trace:
#0 .../proto-bug-repro/test.php(18): Google\Protobuf\Internal\Message->mergeFromJsonString('{"time":"2020-1...')
#1 {main}
  thrown in .../proto-bug-repro/test.php on line 18

[haberman]

I'm glad to hear it's no longer a segfault.

The parse error you see is intentional: the code is written to only accept time zone offsets ending in :00. When I wrote the code I believed that this was according to spec, however I may have been mistaken. I'll circle back with the team to discuss when we're all back at work next week.

[haberman]

The conclusion is that we should accept any minute 0-59 in the offset. I'll write a fix for this.