Skip to content

PHPC-741: Consistent exceptions for BSON init methods #346

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 12 commits into from
Jul 14, 2016
Merged

PHPC-741: Consistent exceptions for BSON init methods #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5c65399
Define ZSTR_VAL() for PHP 5 and remove phongo_str() macro
jmikola Jul 13, 2016
7d3dadb
PHPC-741: Consistent exceptions for Binary init methods
jmikola Jul 13, 2016
e38c21d
PHPC-741: Consistent exceptions for Decimal128 init methods
jmikola Jul 13, 2016
3cd5857
PHPC-741: Consistent exceptions for Javascript init methods
jmikola Jul 13, 2016
6825553
Rename MaxKey and MinKey serialization tests
jmikola Jul 13, 2016
04765b4
PHPC-741: Consistent exceptions for ObjectID init methods
jmikola Jul 13, 2016
160c65d
PHPC-741: Consistent exceptions for Regex init methods
jmikola Jul 13, 2016
9c514b5
PHPC-741: Consistent exceptions for Timestamp init methods
jmikola Jul 13, 2016
f1e057d
PHPC-741: Consistent exceptions for UTCDateTime init methods
jmikola Jul 13, 2016
40f2603
Remove unused STRTOLL() macro
jmikola Jul 13, 2016
b6754f6
PHPC-721: Do not allow null bytes in Javascript code
jmikola Jul 14, 2016
66822bf
PHPC-724: Do not allow null bytes in Regex pattern or flags
jmikola Jul 14, 2016
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
13 changes: 1 addition & 12 deletions phongo_compat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,6 @@
# define ARG_UNUSED
#endif

#ifdef HAVE_ATOLL
# define STRTOLL(s) atoll(s)
#else
# if defined(PHP_WIN32)
# define STRTOLL(s) _atoi64(s)
# else
# define STRTOLL(s) strtoll(s, NULL, 10)
# endif
#endif

#ifndef php_ignore_value
# if defined(__GNUC__) && __GNUC__ >= 4
# define php_ignore_value(x) (({ __typeof__ (x) __x = (x); (void) __x; }))
Expand All @@ -116,7 +106,6 @@
# error Unsupported architecture (integers are neither 32-bit nor 64-bit)
#endif
# define SIZEOF_PHONGO_LONG SIZEOF_ZEND_LONG
# define phongo_str(str) (str)->val
# define phongo_create_object_retval zend_object*
# define PHONGO_ALLOC_OBJECT_T(_obj_t, _class_type) (_obj_t *)ecalloc(1, sizeof(_obj_t)+zend_object_properties_size(_class_type))
# define PHONGO_TSRMLS_FETCH_FROM_CTX(user_data)
Expand Down Expand Up @@ -147,7 +136,7 @@
# define phongo_long long
# define PHONGO_LONG_FORMAT "ld"
# define SIZEOF_PHONGO_LONG SIZEOF_LONG
# define phongo_str(str) str
# define ZSTR_VAL(str) str
Copy link
Member Author

Choose a reason for hiding this comment

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

A number of the exception messages for PHPC-741 involved reporting zend_class_entry->name. On PHP 7, this required accessing zend_string->val. Digging a little deeper, I found that we were inconsistently using either the phongo_str() macro or using #if blocks to conditionally use ZSTR_VAL() for PHP 7. Simply backporting ZSTR_VAL() to PHP 5.x allowed me to remove our custom macro and delete those #if blocks.

# define phongo_create_object_retval zend_object_value
# define PHONGO_ALLOC_OBJECT_T(_obj_t, _class_type) (_obj_t *)ecalloc(1, sizeof(_obj_t))
# define PHONGO_TSRMLS_FETCH_FROM_CTX(user_data) TSRMLS_FETCH_FROM_CTX(user_data)
Expand Down
30 changes: 15 additions & 15 deletions php_phongo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ static void php_phongo_log(mongoc_log_level_t log_level, const char *log_domain,
time(&t);
dt = php_format_date((char *)"Y-m-d\\TH:i:sP", strlen("Y-m-d\\TH:i:sP"), t, 0 TSRMLS_CC);

fprintf(MONGODB_G(debug_fd), "[%s] %10s: %-8s> %s\n", phongo_str(dt), log_domain, mongoc_log_level_str(log_level), message);
fprintf(MONGODB_G(debug_fd), "[%s] %10s: %-8s> %s\n", ZSTR_VAL(dt), log_domain, mongoc_log_level_str(log_level), message);
fflush(MONGODB_G(debug_fd));
efree(dt);
} break;
Expand Down Expand Up @@ -1236,7 +1236,7 @@ mongoc_stream_t* phongo_stream_initiator(const mongoc_uri_t *uri, const mongoc_h
zend_restore_error_handling(&error_handling TSRMLS_CC);

if (!stream) {
bson_set_error (error, MONGOC_ERROR_STREAM, MONGOC_ERROR_STREAM_CONNECT, "Failed connecting to '%s:%d': %s", host->host, host->port, phongo_str(errmsg));
bson_set_error (error, MONGOC_ERROR_STREAM, MONGOC_ERROR_STREAM_CONNECT, "Failed connecting to '%s:%d': %s", host->host, host->port, ZSTR_VAL(errmsg));
efree(dsn);
efree(uniqid);
if (errmsg) {
Expand Down Expand Up @@ -2342,11 +2342,11 @@ ZEND_INI_MH(OnUpdateDebug)
MONGODB_G(debug_fd) = NULL;
}

if (!new_value || (new_value && !phongo_str(new_value)[0])
|| strcasecmp("0", phongo_str(new_value)) == 0
|| strcasecmp("off", phongo_str(new_value)) == 0
|| strcasecmp("no", phongo_str(new_value)) == 0
|| strcasecmp("false", phongo_str(new_value)) == 0
if (!new_value || (new_value && !ZSTR_VAL(new_value)[0])
|| strcasecmp("0", ZSTR_VAL(new_value)) == 0
|| strcasecmp("off", ZSTR_VAL(new_value)) == 0
|| strcasecmp("no", ZSTR_VAL(new_value)) == 0
|| strcasecmp("false", ZSTR_VAL(new_value)) == 0
) {
mongoc_log_trace_disable();
mongoc_log_set_handler(NULL, NULL);
Expand All @@ -2359,19 +2359,19 @@ ZEND_INI_MH(OnUpdateDebug)
}


if (strcasecmp(phongo_str(new_value), "stderr") == 0) {
if (strcasecmp(ZSTR_VAL(new_value), "stderr") == 0) {
MONGODB_G(debug_fd) = stderr;
} else if (strcasecmp(phongo_str(new_value), "stdout") == 0) {
} else if (strcasecmp(ZSTR_VAL(new_value), "stdout") == 0) {
MONGODB_G(debug_fd) = stdout;
} else if (
strcasecmp("1", phongo_str(new_value)) == 0
|| strcasecmp("on", phongo_str(new_value)) == 0
|| strcasecmp("yes", phongo_str(new_value)) == 0
|| strcasecmp("true", phongo_str(new_value)) == 0
strcasecmp("1", ZSTR_VAL(new_value)) == 0
|| strcasecmp("on", ZSTR_VAL(new_value)) == 0
|| strcasecmp("yes", ZSTR_VAL(new_value)) == 0
|| strcasecmp("true", ZSTR_VAL(new_value)) == 0
) {
tmp_dir = NULL;
} else {
tmp_dir = phongo_str(new_value);
tmp_dir = ZSTR_VAL(new_value);
}

if (!MONGODB_G(debug_fd)) {
Expand All @@ -2386,7 +2386,7 @@ ZEND_INI_MH(OnUpdateDebug)

fd = php_open_temporary_fd(tmp_dir, prefix, &filename TSRMLS_CC);
if (fd != -1) {
const char *path = phongo_str(filename);
const char *path = ZSTR_VAL(filename);
MONGODB_G(debug_fd) = VCWD_FOPEN(path, "a");
}
efree(filename);
Expand Down
29 changes: 14 additions & 15 deletions src/BSON/Binary.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,12 @@ PHONGO_API zend_class_entry *php_phongo_binary_ce;

zend_object_handlers php_phongo_handler_binary;

/* Initialize the object from a string and return whether it was successful. */
static bool php_phongo_binary_init(php_phongo_binary_t *intern, const char *data, phongo_zpp_char_len data_len, phongo_long type)
/* Initialize the object and return whether it was successful. An exception will
* be thrown on error. */
static bool php_phongo_binary_init(php_phongo_binary_t *intern, const char *data, phongo_zpp_char_len data_len, phongo_long type TSRMLS_DC)
{
if (type < 0 || type > UINT8_MAX) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected type to be an unsigned 8-bit integer, %" PHONGO_LONG_FORMAT " given", type);
Copy link
Member Author

Choose a reason for hiding this comment

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

This message was previously only used in the constructor; however, the exception may now be thrown from __set_state() or unserialize(). What do you think about putting the class name in the message?

I've started doing this with other new messages (e.g. Javascript now allowing null bytes in its code argument) for added clarity, but did not want to change existing messages before discussing.

Copy link
Member Author

@jmikola jmikola Jul 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify, my main concern was with unserialize(), as I expected that would be the only function in the stack trace. After further testing, it appears that the appropriate __wakeup() method is included in the backtrace as "[internal function]". I tested this by unserializing a Javascript object with an invalid Regex in its scope document, just to see how nesting is handled.

<?php

unserialize('O:23:"MongoDB\BSON\Javascript":2:{s:4:"code";s:33:"function foo(bar) { return bar; }";s:5:"scope";O:8:"stdClass":1:{s:1:"r";O:18:"MongoDB\BSON\Regex":2:{s:7:"pattern";s:6:"regexp";s:5:"flags";s:1:"'. "\0" .'";}}}');

Output:

Fatal error: Uncaught exception 'MongoDB\Driver\Exception\InvalidArgumentException' with message 'MongoDB\BSON\Regex flags should not contain null bytes' in tests/bson/bson-regex-serialization_error-002.php:15
Stack trace:
#0 [internal function]: MongoDB\BSON\Regex->__wakeup()
#1 tests/bson/bson-regex-serialization_error-002.php(15): unserialize('O:23:"MongoDB\\B...')
#2 {main}
  thrown in tests/bson/bson-regex-serialization_error-002.php on line 15

Given this behavior, I think we're OK with not having the class name in every message.

return false;
}

Expand All @@ -60,24 +62,27 @@ static bool php_phongo_binary_init(php_phongo_binary_t *intern, const char *data
return true;
}

/* Initialize the object from a HashTable and return whether it was successful. */
static bool php_phongo_binary_init_from_hash(php_phongo_binary_t *intern, HashTable *props)
/* Initialize the object from a HashTable and return whether it was successful.
* An exception will be thrown on error. */
static bool php_phongo_binary_init_from_hash(php_phongo_binary_t *intern, HashTable *props TSRMLS_DC)
{
#if PHP_VERSION_ID >= 70000
zval *data, *type;

if ((data = zend_hash_str_find(props, "data", sizeof("data")-1)) && Z_TYPE_P(data) == IS_STRING &&
(type = zend_hash_str_find(props, "type", sizeof("type")-1)) && Z_TYPE_P(type) == IS_LONG) {
return php_phongo_binary_init(intern, Z_STRVAL_P(data), Z_STRLEN_P(data), Z_LVAL_P(type));
return php_phongo_binary_init(intern, Z_STRVAL_P(data), Z_STRLEN_P(data), Z_LVAL_P(type) TSRMLS_CC);
}
#else
zval **data, **type;

if (zend_hash_find(props, "data", sizeof("data"), (void**) &data) == SUCCESS && Z_TYPE_PP(data) == IS_STRING &&
zend_hash_find(props, "type", sizeof("type"), (void**) &type) == SUCCESS && Z_TYPE_PP(type) == IS_LONG) {
return php_phongo_binary_init(intern, Z_STRVAL_PP(data), Z_STRLEN_PP(data), Z_LVAL_PP(type));
return php_phongo_binary_init(intern, Z_STRVAL_PP(data), Z_STRLEN_PP(data), Z_LVAL_PP(type) TSRMLS_CC);
}
#endif

phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "%s initialization requires \"data\" string and \"type\" integer fields", ZSTR_VAL(php_phongo_binary_ce->name));
return false;
}

Expand All @@ -101,9 +106,7 @@ PHP_METHOD(Binary, __construct)
}
zend_restore_error_handling(&error_handling TSRMLS_CC);

if (!php_phongo_binary_init(intern, data, data_len, type)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected type to be an unsigned 8-bit integer, %" PHONGO_LONG_FORMAT " given", type);
}
php_phongo_binary_init(intern, data, data_len, type TSRMLS_CC);
}
/* }}} */

Expand All @@ -124,9 +127,7 @@ PHP_METHOD(Binary, __set_state)
intern = Z_BINARY_OBJ_P(return_value);
props = Z_ARRVAL_P(array);

if (!php_phongo_binary_init_from_hash(intern, props)) {
php_error(E_ERROR, "Invalid serialization data for Binary object");
}
php_phongo_binary_init_from_hash(intern, props TSRMLS_CC);
}
/* }}} */

Expand All @@ -144,9 +145,7 @@ PHP_METHOD(Binary, __wakeup)
intern = Z_BINARY_OBJ_P(getThis());
props = zend_std_get_properties(getThis() TSRMLS_CC);

if (!php_phongo_binary_init_from_hash(intern, props)) {
php_error(E_ERROR, "Invalid serialization data for Binary object");
}
php_phongo_binary_init_from_hash(intern, props TSRMLS_CC);
}
/* }}} */

Expand Down
39 changes: 19 additions & 20 deletions src/BSON/Decimal128.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,34 +46,39 @@ PHONGO_API zend_class_entry *php_phongo_decimal128_ce;

zend_object_handlers php_phongo_handler_decimal128;

/* Initialize the object from a string and return whether it was successful. */
static bool php_phongo_decimal128_init(php_phongo_decimal128_t *intern, const char *value)
/* Initialize the object and return whether it was successful. An exception will
* be thrown on error. */
static bool php_phongo_decimal128_init(php_phongo_decimal128_t *intern, const char *value TSRMLS_DC)
{
if (bson_decimal128_from_string(value, &intern->decimal)) {
intern->initialized = true;

return true;
if (!bson_decimal128_from_string(value, &intern->decimal)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error parsing decimal string: %s", value);
Copy link
Member Author

@jmikola jmikola Jul 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed that this differs from HHVM's exception message (see: here). We must have missed the different when originally reviewing PHPC-619's PR.

Let's take this opportunity to settle on a consistent format.

return false;
}

return false;
intern->initialized = true;

return true;
}

/* Initialize the object from a HashTable and return whether it was successful. */
static bool php_phongo_decimal128_init_from_hash(php_phongo_decimal128_t *intern, HashTable *props)
/* Initialize the object from a HashTable and return whether it was successful.
* An exception will be thrown on error. */
static bool php_phongo_decimal128_init_from_hash(php_phongo_decimal128_t *intern, HashTable *props TSRMLS_DC)
{
#if PHP_VERSION_ID >= 70000
zval *dec;

if ((dec = zend_hash_str_find(props, "dec", sizeof("dec")-1)) && Z_TYPE_P(dec) == IS_STRING) {
return php_phongo_decimal128_init(intern, Z_STRVAL_P(dec));
return php_phongo_decimal128_init(intern, Z_STRVAL_P(dec) TSRMLS_CC);
}
#else
zval **dec;

if (zend_hash_find(props, "dec", sizeof("dec"), (void**) &dec) == SUCCESS && Z_TYPE_PP(dec) == IS_STRING) {
return php_phongo_decimal128_init(intern, Z_STRVAL_PP(dec));
return php_phongo_decimal128_init(intern, Z_STRVAL_PP(dec) TSRMLS_CC);
}
#endif

phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "%s initialization requires \"dec\" string field", ZSTR_VAL(php_phongo_decimal128_ce->name));
return false;
}

Expand All @@ -96,9 +101,7 @@ PHP_METHOD(Decimal128, __construct)
}
zend_restore_error_handling(&error_handling TSRMLS_CC);

if (!php_phongo_decimal128_init(intern, value)) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error parsing decimal string: %s", value);
}
php_phongo_decimal128_init(intern, value TSRMLS_CC);
}
/* }}} */

Expand All @@ -119,9 +122,7 @@ PHP_METHOD(Decimal128, __set_state)
intern = Z_DECIMAL128_OBJ_P(return_value);
props = Z_ARRVAL_P(array);

if (!php_phongo_decimal128_init_from_hash(intern, props)) {
php_error(E_ERROR, "Invalid serialization data for Decimal128 object");
}
php_phongo_decimal128_init_from_hash(intern, props TSRMLS_CC);
}
/* }}} */

Expand Down Expand Up @@ -158,9 +159,7 @@ PHP_METHOD(Decimal128, __wakeup)
intern = Z_DECIMAL128_OBJ_P(getThis());
props = zend_std_get_properties(getThis() TSRMLS_CC);

if (!php_phongo_decimal128_init_from_hash(intern, props)) {
php_error(E_ERROR, "Invalid serialization data for Decimal128 object");
}
php_phongo_decimal128_init_from_hash(intern, props TSRMLS_CC);
}
/* }}} */

Expand Down
23 changes: 14 additions & 9 deletions src/BSON/Javascript.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,17 @@ PHONGO_API zend_class_entry *php_phongo_javascript_ce;

zend_object_handlers php_phongo_handler_javascript;

/* Initialize the object from a string and return whether it was successful. */
/* Initialize the object and return whether it was successful. An exception will
* be thrown on error. */
static bool php_phongo_javascript_init(php_phongo_javascript_t *intern, const char *code, phongo_zpp_char_len code_len, zval *scope TSRMLS_DC)
{
if (scope && Z_TYPE_P(scope) != IS_OBJECT && Z_TYPE_P(scope) != IS_ARRAY && Z_TYPE_P(scope) != IS_NULL) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Expected scope to be array or object, %s given", zend_get_type_by_const(Z_TYPE_P(scope)));
return false;
}

if (strlen(code) != code_len) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Code cannot contain null bytes");
return false;
}

Expand All @@ -66,7 +73,8 @@ static bool php_phongo_javascript_init(php_phongo_javascript_t *intern, const ch
return true;
}

/* Initialize the object from a HashTable and return whether it was successful. */
/* Initialize the object from a HashTable and return whether it was successful.
* An exception will be thrown on error. */
static bool php_phongo_javascript_init_from_hash(php_phongo_javascript_t *intern, HashTable *props TSRMLS_DC)
{
#if PHP_VERSION_ID >= 70000
Expand All @@ -86,6 +94,8 @@ static bool php_phongo_javascript_init_from_hash(php_phongo_javascript_t *intern
return php_phongo_javascript_init(intern, Z_STRVAL_PP(code), Z_STRLEN_PP(code), tmp TSRMLS_CC);
}
#endif

phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "%s initialization requires \"code\" string field", ZSTR_VAL(php_phongo_javascript_ce->name));
return false;
}

Expand Down Expand Up @@ -131,9 +141,7 @@ PHP_METHOD(Javascript, __set_state)
intern = Z_JAVASCRIPT_OBJ_P(return_value);
props = Z_ARRVAL_P(array);

if (!php_phongo_javascript_init_from_hash(intern, props TSRMLS_CC)) {
php_error(E_ERROR, "Invalid serialization data for Javascript object");
}
php_phongo_javascript_init_from_hash(intern, props TSRMLS_CC);
}
/* }}} */

Expand All @@ -151,10 +159,7 @@ PHP_METHOD(Javascript, __wakeup)
intern = Z_JAVASCRIPT_OBJ_P(getThis());
props = zend_std_get_properties(getThis() TSRMLS_CC);

if (!php_phongo_javascript_init_from_hash(intern, props TSRMLS_CC)) {
php_error(E_ERROR, "Invalid serialization data for Javascript object");
} else {
}
php_phongo_javascript_init_from_hash(intern, props TSRMLS_CC);
}
/* }}} */

Expand Down
Loading