Skip to content

Conversation

MarcusSchwarz
Copy link

No description provided.

result->no_language = string->no_language;
result->no_encoding = string->no_encoding;
mbfl_memory_device_init(&pc.device, width, 0);
mbfl_memory_device_init(&pc.device, string->len, 0);
Copy link
Member

Choose a reason for hiding this comment

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

This should use width or string->len, whichever is lower.

@nikic
Copy link
Member

nikic commented Jun 28, 2018

Merged as bf5a802 into 7.1+. Thanks!

@nikic nikic closed this Jun 28, 2018
@cmb69
Copy link
Member

cmb69 commented Jun 28, 2018

Is it really okay to rely on the zend_long to int conversion in the first place? To my knowledge, this is implementation defined behavior.

Also, the bug report claims that the issue occurs for $width > 2147483647, but the PHPT uses $width == 2147483647.

@nikic
Copy link
Member

nikic commented Jun 28, 2018

@cmb69 This PR fixes the issue of unnecessary over-allocation if the trim width exceeds the string length. The type conversion is a separate problem and already fixed in PHP 7.3 by the migration to size_t.

@cmb69
Copy link
Member

cmb69 commented Jun 28, 2018

@nikic Thanks! However, merging of the PR closed bug 76532 which is (also) about the integer overflow. For PHP 7.1 and 7.2, it might be sensible to check for width < 0 || width > INT_MAX) here,

@nikic
Copy link
Member

nikic commented Jun 28, 2018

@cmb69 This PR fixes the practical concern, and fully correct behavior is not possible on older versions, as the libmbfl support is not there. Making it fail nicer is IMHO a waste of time. However, feel free to add a check (just need to make sure it's not merged into master).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants