Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 18, 2016
+22 −13
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/BSON/Timestamp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
if (increment < 0 || increment > UINT32_MAX) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected increment to be an unsigned 32-bit integer, %" PHONGO_LONG_FORMAT " given", increment);
Expand All @@ -66,8 +66,8 @@ static bool php_phongo_timestamp_init(php_phongo_timestamp_t *intern, phongo_lon
return false;
}

intern->increment = increment;
intern->timestamp = timestamp;
intern->increment = (uint32_t) increment;
intern->timestamp = (uint32_t) timestamp;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to worry about endianness here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

intern->initialized = true;

return true;
Expand Down Expand Up @@ -413,8 +413,8 @@ HashTable *php_phongo_timestamp_get_properties(zval *object TSRMLS_DC) /* {{{ */
return props;
}

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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#if PHP_VERSION_ID >= 70000
{
Expand Down
8 changes: 4 additions & 4 deletions tests/bson/bson-timestamp-set_state_error-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
MongoDB\BSON\Timestamp::__set_state(['increment' => 1234, 'timestamp' => -1]);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

echo throws(function() {
MongoDB\BSON\Timestamp::__set_state(['increment' => 1234, 'timestamp' => -2147483648]);
MongoDB\BSON\Timestamp::__set_state(['increment' => 1234, 'timestamp' => -2147483647]);
}, 'MongoDB\Driver\Exception\InvalidArgumentException'), "\n";

?>
Expand All @@ -28,9 +28,9 @@ echo throws(function() {
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected increment to be an unsigned 32-bit integer, -1 given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected increment to be an unsigned 32-bit integer, -2147483648 given
Expected increment to be an unsigned 32-bit integer, -2147483647 given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected timestamp to be an unsigned 32-bit integer, -1 given
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Expected timestamp to be an unsigned 32-bit integer, -2147483648 given
Expected timestamp to be an unsigned 32-bit integer, -2147483647 given
===DONE===
17 changes: 13 additions & 4 deletions tests/bson/bug0592.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ foreach ($tests as $json) {
===DONE===
<?php exit(0); ?>
--EXPECTF--
%a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reading of /etc/issue and something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Test { "x": { "$numberLong": "-2147483648" }}
object(stdClass)#%d (%d) {
["x"]=>
Expand All @@ -44,15 +45,22 @@ object(stdClass)#%d (%d) {
int(2147483647)
}

Test { "x": { "$numberLong": "4294967294" }}
[%s] PHONGO-BSON: WARNING > Integer overflow detected on your platform: 4294967294
object(stdClass)#%d (%d) {
["x"]=>
string(10) "4294967294"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Test { "x": { "$numberLong": "4294967295" }}
[%s] PHONGO-BSON: WARNING > ENTRY: Integer overflow detected on your platform: 4294967295
[%s] PHONGO-BSON: WARNING > Integer overflow detected on your platform: 4294967295
object(stdClass)#%d (%d) {
["x"]=>
string(10) "4294967295"
}

Test { "x": { "$numberLong": "9223372036854775807" }}
[%s] PHONGO-BSON: WARNING > ENTRY: Integer overflow detected on your platform: 9223372036854775807
[%s] PHONGO-BSON: WARNING > Integer overflow detected on your platform: 9223372036854775807
object(stdClass)#%d (%d) {
["x"]=>
string(19) "9223372036854775807"
Expand All @@ -71,20 +79,21 @@ object(stdClass)#%d (%d) {
}

Test { "longFieldName": { "$numberLong": "4294967294" }}
[%s] PHONGO-BSON: WARNING > Integer overflow detected on your platform: 4294967294
object(stdClass)#%d (%d) {
["longFieldName"]=>
string(10) "4294967294"
}

Test { "longFieldName": { "$numberLong": "4294967295" }}
[%s] PHONGO-BSON: WARNING > ENTRY: Integer overflow detected on your platform: 4294967295
[%s] PHONGO-BSON: WARNING > Integer overflow detected on your platform: 4294967295
object(stdClass)#%d (%d) {
["longFieldName"]=>
string(10) "4294967295"
}

Test { "longFieldName": { "$numberLong": "9223372036854775807" }}
[%s] PHONGO-BSON: WARNING > ENTRY: Integer overflow detected on your platform: 9223372036854775807
[%s] PHONGO-BSON: WARNING > Integer overflow detected on your platform: 9223372036854775807
object(stdClass)#%d (%d) {
["longFieldName"]=>
string(19) "9223372036854775807"
Expand Down