Skip to content

Small fixes for Timestamp and UTCDateTime #857

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
jmikola merged 2 commits into from
Jun 22, 2018
Merged

Small fixes for Timestamp and UTCDateTime #857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e584ee8
Update bson_ascii_strtoll() usage in Timestamp and UTCDateTime
jmikola Jun 21, 2018
ae98012
Use static string buffer for UTCDateTime::__toString()
jmikola Jun 21, 2018
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
11 changes: 2 additions & 9 deletions src/BSON/Timestamp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,8 @@ static bool php_phongo_timestamp_init_from_string(php_phongo_timestamp_t* intern
int64_t increment, timestamp;
char* endptr = NULL;

errno = 0;

/* errno will set errno if conversion fails; however, we do not need to
* specify the type of error.
*
* Note: bson_ascii_strtoll() does not properly detect out-of-range values
* (see: CDRIVER-1377). strtoll() would be preferable, but it is not
* available on all platforms (e.g. HP-UX), and atoll() provides no error
* reporting at all. */
/* bson_ascii_strtoll() sets errno if conversion fails. If conversion
* succeeds, we still want to ensure that the entire string was parsed. */

increment = bson_ascii_strtoll(s_increment, &endptr, 10);

Expand Down
20 changes: 7 additions & 13 deletions src/BSON/UTCDateTime.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,11 @@ static bool php_phongo_utcdatetime_init_from_string(php_phongo_utcdatetime_t* in
int64_t milliseconds;
char* endptr = NULL;

errno = 0;
/* bson_ascii_strtoll() sets errno if conversion fails. If conversion
* succeeds, we still want to ensure that the entire string was parsed. */

milliseconds = bson_ascii_strtoll(s_milliseconds, &endptr, 10);

/* errno will set errno if conversion fails; however, we do not need to
* specify the type of error.
*
* Note: bson_ascii_strtoll() does not properly detect out-of-range values
* (see: CDRIVER-1377). strtoll() would be preferable, but it is not
* available on all platforms (e.g. HP-UX), and atoll() provides no error
* reporting at all. */
if (errno || (endptr && endptr != ((const char*) s_milliseconds + s_milliseconds_len))) {
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "Error parsing \"%s\" as 64-bit integer for %s initialization", s_milliseconds, ZSTR_VAL(php_phongo_utcdatetime_ce->name));
return false;
Expand Down Expand Up @@ -223,18 +217,18 @@ static PHP_METHOD(UTCDateTime, __set_state)
static PHP_METHOD(UTCDateTime, __toString)
{
php_phongo_utcdatetime_t* intern;
char* tmp;
int tmp_len;
char s_milliseconds[24];
int s_milliseconds_len;

intern = Z_UTCDATETIME_OBJ_P(getThis());

if (zend_parse_parameters_none() == FAILURE) {
return;
}

tmp_len = spprintf(&tmp, 0, "%" PRId64, intern->milliseconds);
PHONGO_RETVAL_STRINGL(tmp, tmp_len);
efree(tmp);
s_milliseconds_len = snprintf(s_milliseconds, sizeof(s_milliseconds), "%" PRId64, intern->milliseconds);

PHONGO_RETVAL_STRINGL(s_milliseconds, s_milliseconds_len);
Copy link
Contributor

@derickr derickr Jun 22, 2018

Choose a reason for hiding this comment

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

What is the reason for making this change? The use of snprintf is potentially unsafe due to strange \0 termination rules, and spprintf is preferred.

Copy link
Member Author

@jmikola jmikola Jun 22, 2018

Choose a reason for hiding this comment

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

jsonSerialize(), which is effectively the same thing, was already using snprintf() with a static buffer (24 is sufficient to hold INT64_MIN and the null byte).

I don't follow why this would be potentially unsafe. My understanding is that snprintf() always adds a null byte (at the expense of the string it's printing if it were to approach the buffer size). See: https://stackoverflow.com/q/7706936/162228. This makes it safer than sprintf() (risk of no termination) and comparable to spprintf(), which would also always terminate the allocated string.

} /* }}} */

/* {{{ proto DateTime MongoDB\BSON\UTCDateTime::toDateTime()
Expand Down