PHPC-788: Timestamp on 32-bit platforms breaks #435

derickr · 2016-10-13T12:01:23Z

No description provided.

derickr · 2016-10-13T12:02:19Z

src/BSON/Timestamp.c

@@ -54,7 +54,7 @@ zend_object_handlers php_phongo_handler_timestamp;

 /* Initialize the object and return whether it was successful. An exception will
 * be thrown on error. */
-static bool php_phongo_timestamp_init(php_phongo_timestamp_t *intern, phongo_long increment, phongo_long timestamp TSRMLS_DC)
+static bool php_phongo_timestamp_init(php_phongo_timestamp_t *intern, int64_t increment, int64_t timestamp TSRMLS_DC)


This needs to be int64_t, as that's what the original arguments that were passed in here are as well. phongo_long is also not 64-bit on 32-bit platforms, and hence the comparison with UINT32_MIN would fail for some values.

I assume you mean the < 0 comparisons would fail?

Since php_phongo_timestamp_init_from_string() was always parsing the strings into int64_t values, I suppose there was possible degredation there. The other code path is php_phongo_timestamp_init_from_hash() picking up IS_LONG values. In that case, would a negative int32_t value expand to a negative int64_t value (essentially the sign bit relocates) and still yield an exception?

Yes, the < 0 would fail. And a sign bit doesn't change when you cast.

derickr · 2016-10-13T12:03:07Z

src/BSON/Timestamp.c

-	s_increment_len = snprintf(s_increment, sizeof(s_increment), "%" PRId64, intern->increment);
-	s_timestamp_len = snprintf(s_timestamp, sizeof(s_timestamp), "%" PRId64, intern->timestamp);
+	s_increment_len = snprintf(s_increment, sizeof(s_increment), "%" PRIu32, intern->increment);
+	s_timestamp_len = snprintf(s_timestamp, sizeof(s_timestamp), "%" PRIu32, intern->timestamp);


The value in "increment" and "timestamp" is 32bit unsigned, so using a 64bit formatting would add a random 32bit value to the start of each formatted string, resulting in test failures.

derickr · 2016-10-13T12:04:38Z

tests/bson/bson-timestamp-set_state_error-002.phpt

@@ -10,15 +10,15 @@ echo throws(function() {
 }, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

 echo throws(function() {
-    MongoDB\BSON\Timestamp::__set_state(['increment' => -2147483648, 'timestamp' => 5678]);
+    MongoDB\BSON\Timestamp::__set_state(['increment' => -2147483647, 'timestamp' => 5678]);


PHP's parser parses "-" and "2147483648" separately. And as "2147483648" is over the int-32 limit, it gets converted to a double. -2147483647 is therefore the highest 32bit number PHP itself can represent.

In that case, this isn't really testing a boundary condition, is it? I was testing -2147483648 because it should have been equal to 4294967295 in unsigned form.

I'd be OK with remove these test cases unless you really want to keep them.

It's still a boundary check. Just one for PHP. I'll just keep it.

derickr · 2016-10-13T12:04:46Z

tests/bson/bug0592.phpt

+  ["x"]=>
+  string(10) "4294967294"
+}
+


This test had missing output sections, and the warning formatting was wrong too. This test was always broken.

jmikola

LGTM with some questions and possible test changes.

jmikola · 2016-10-13T15:30:18Z

src/BSON/Timestamp.c

@@ -54,7 +54,7 @@ zend_object_handlers php_phongo_handler_timestamp;

 /* Initialize the object and return whether it was successful. An exception will
 * be thrown on error. */
-static bool php_phongo_timestamp_init(php_phongo_timestamp_t *intern, phongo_long increment, phongo_long timestamp TSRMLS_DC)
+static bool php_phongo_timestamp_init(php_phongo_timestamp_t *intern, int64_t increment, int64_t timestamp TSRMLS_DC)


I assume you mean the < 0 comparisons would fail?

Since php_phongo_timestamp_init_from_string() was always parsing the strings into int64_t values, I suppose there was possible degredation there. The other code path is php_phongo_timestamp_init_from_hash() picking up IS_LONG values. In that case, would a negative int32_t value expand to a negative int64_t value (essentially the sign bit relocates) and still yield an exception?

jmikola · 2016-10-13T15:31:07Z

src/BSON/Timestamp.c

-	intern->increment = increment;
-	intern->timestamp = timestamp;
+	intern->increment = (uint32_t) increment;
+	intern->timestamp = (uint32_t) timestamp;


Do we have to worry about endianness here?

jmikola · 2016-10-13T15:34:23Z

tests/bson/bson-timestamp-set_state_error-002.phpt

@@ -10,15 +10,15 @@ echo throws(function() {
 }, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

 echo throws(function() {
-    MongoDB\BSON\Timestamp::__set_state(['increment' => -2147483648, 'timestamp' => 5678]);
+    MongoDB\BSON\Timestamp::__set_state(['increment' => -2147483647, 'timestamp' => 5678]);


In that case, this isn't really testing a boundary condition, is it? I was testing -2147483648 because it should have been equal to 4294967295 in unsigned form.

I'd be OK with remove these test cases unless you really want to keep them.

jmikola · 2016-10-13T15:36:39Z

tests/bson/bug0592.phpt

@@ -32,6 +32,7 @@ foreach ($tests as $json) {
 ===DONE===
 <?php exit(0); ?>
 --EXPECTF--
+%a


What is being printed before the first test case? It may make more sense to simply enable and disable mongodb.debug=stderr around the foreach that executes all of the test cases, instead of having the INI setting on for the entire process. We really only want to check for the overflow warnings.

The reading of /etc/issue and something.

I'll make the debug=stderr changes, but the other PR will just remove them again...

If you're going to clean this up in the other PR, don't worry about duplicating that work here.

jmikola

LGTM. mongodb.debug will be cleaned up in #436.

derickr commented Oct 13, 2016

View reviewed changes

jmikola approved these changes Oct 13, 2016

View reviewed changes

jmikola mentioned this pull request Oct 13, 2016

PHPC-313: BSON should throw when encountering 64-bit integer on 32-bit platform #436

Merged

PHPC-788: Timestamp on 32-bit platforms breaks

eb35da1

derickr force-pushed the PHPC-788-timestamp-32bit branch from 829a08c to eb35da1 Compare October 18, 2016 09:52

derickr merged commit eb35da1 into mongodb:master Oct 18, 2016

derickr added a commit that referenced this pull request Oct 18, 2016

Merged pull request #435

3849719

PHPC-788: Timestamp on 32-bit platforms breaks #435

PHPC-788: Timestamp on 32-bit platforms breaks #435

Uh oh!

Conversation

derickr commented Oct 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!