Skip to content

Fix #75857#3078

Closed
carusogabriel wants to merge 1 commit intophp:PHP-7.1from
carusogabriel:fix-75857
Closed

Fix #75857#3078
carusogabriel wants to merge 1 commit intophp:PHP-7.1from
carusogabriel:fix-75857

Conversation

@carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Feb 5, 2018 •
edited
Loading

This is a failing test case for https://bugs.php.net/bug.php?id=75857

The error is maybe in:

php-src/ext/date/php_date.c

Lines 1205 to 1210 in 39c5857

case 'e': if (!localtime) {
length = slprintf(buffer, 32, "%s", "UTC");
} else {
switch (t->zone_type) {
case TIMELIB_ZONETYPE_ID:
length = slprintf(buffer, 32, "%s", t->tz_info->name);

Should we simply change the len size to 33 or so?

Also, was reported in 7.2.2RC1, but should we target 7.1?

@cmb69
Copy link
Member

cmb69 commented Feb 6, 2018

If a test is supposed to fail, it should have an XFAIL section.

Wrt. the actual bugfix: I wonder why there are hard-coded lengths at all. It seems to me that sizeof(buffer) would be preferable.

@carusogabriel
Copy link
Contributor Author

carusogabriel commented Feb 6, 2018

@cmb69 Sorry, I didn't put it to fail and we discuss a solution, but sorry, I'll put next time.

It seems to me that sizeof(buffer) would be preferable.

Hm, makes sense, like

php-src/ext/interbase/ibase_query.c

Lines 1335 to 1345 in c73e8a0

l = slprintf(string_data, sizeof(string_data), "%" LL_MASK "d", *(ISC_INT64 *) data);
ZVAL_STRINGL(val,string_data,l);
} else {
ISC_INT64 n = *(ISC_INT64 *) data, f = scales[-scale];
if (n >= 0) {
l = slprintf(string_data, sizeof(string_data), "%" LL_MASK "d.%0*" LL_MASK "d", n / f, -scale, n % f);
} else if (n <= -f) {
l = slprintf(string_data, sizeof(string_data), "%" LL_MASK "d.%0*" LL_MASK "d", n / f, -scale, -n % f);
} else {
l = slprintf(string_data, sizeof(string_data), "-0.%0*" LL_MASK "d", -scale, -n % f);

?

@krakjoe
Copy link
Member

krakjoe commented Feb 8, 2018

Target the lowest applicable branch that exhibits the behaviour.

Also, the test doesn't seem to fail anymore, so what's the current status of this patch ?

@krakjoe krakjoe added the Bug label Feb 8, 2018
@carusogabriel
Copy link
Contributor Author

carusogabriel commented Feb 8, 2018

@krakjoe

Target the lowest applicable branch that exhibits the behavior.

Targeting 7.1...

Also, the test doesn't seem to fail anymore, so what's the current status of this patch?

I followed @cmb69 advice and change 32 (and others hard-coded values) to sizeof(buffer), and this fixed #75857

@carusogabriel carusogabriel changed the base branch from master to PHP-7.1 February 8, 2018 12:05
@carusogabriel
Fix #75857
95f4ca0 
Use sizeof buffer instead of hard-coded lenghts
@carusogabriel carusogabriel changed the title Add falling test case for #75857 Fix #75857 Feb 8, 2018
@php-pulls
Copy link

php-pulls commented Feb 11, 2018

Comment on behalf of cmb at php.net:

Thanks, @carusogabriel! Applied via f706937.

@php-pulls php-pulls closed this Feb 11, 2018
@carusogabriel carusogabriel deleted the fix-75857 branch February 11, 2018 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nikic nikic nikic approved these changes

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@carusogabriel @cmb69 @krakjoe @php-pulls @nikic