Skip to content

Allow empty needles in mbstring functions #4977

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Allow empty needles in mbstring functions #4977

wants to merge 3 commits into from

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Dec 7, 2019

These pass on my machine without any test fixes ... So I don't know what to think about this.

@Girgias
Copy link
Member Author

Girgias commented Dec 7, 2019

I think this is just causing the underlying C lib to return an error code which is transmitted via the normal FALSE return.

@nikic
Copy link
Member

nikic commented Dec 7, 2019

Right, the mbfl code will also need to be adjusted:

php-src/ext/mbstring/libmbfl/mbfl/mbfilter.c

Lines 858 to 861 in 1d69bf1

if (needle_u8->len < 1) {
result = (size_t) -8;
goto out;
}

@cmb69
Copy link
Member

cmb69 commented Dec 7, 2019 •
edited
Loading

It seems to me that would need more adjustments, e.g.

php-src/ext/mbstring/libmbfl/mbfl/mbfilter.c

Line 878 in 1d69bf1

for (i = 0; i < needle_u8_len - 1; ++i) {
would underflow.

PS: Sorry, that's nonsense. Just disregard.

@Girgias Girgias force-pushed the allow-empty-needle-mbstring branch from 49ab8a4 to 12cb36d Compare December 7, 2019 15:28
@Girgias Girgias mentioned this pull request Dec 7, 2019
Closed
nikic
nikic reviewed Dec 11, 2019
View reviewed changes
@Girgias Girgias force-pushed the allow-empty-needle-mbstring branch from 12cb36d to bfad1ff Compare December 11, 2019 21:26
@Girgias
Copy link
Member Author

Girgias commented Dec 13, 2019

The Windows test failure seems to be related but I can't see how

@cmb69
Copy link
Member

cmb69 commented Dec 13, 2019

I think that test failure is intermittent.

nikic
nikic reviewed Dec 13, 2019
View reviewed changes
@nikic
Copy link
Member

nikic commented Dec 13, 2019

Can you please also add some tests for empty string + out of bounds offset? Not necessarily for everything, but at least one forward and reverse case.

@Girgias Girgias force-pushed the allow-empty-needle-mbstring branch from bfad1ff to d860e06 Compare December 13, 2019 16:59
nikic
nikic reviewed Dec 13, 2019
View reviewed changes
@Girgias Girgias requested a review from nikic December 30, 2019 18:59
nikic
nikic reviewed Jan 1, 2020
View reviewed changes
nikic
nikic reviewed Jan 3, 2020
View reviewed changes
@Girgias Girgias force-pushed the allow-empty-needle-mbstring branch from a037557 to 290e9f1 Compare January 7, 2020 16:26
Girgias
Girgias commented Jan 7, 2020
View reviewed changes
ext/mbstring/libmbfl/mbfl/mbfilter.c
Comment on lines +881 to +883
if (offset < 0) {
result = haystack_length + offset;
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I move the if (offset < 0) condition out? As it's the same for both now.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable.

@Girgias
Copy link
Member Author

Girgias commented Jan 7, 2020

Azure pipeline failure is unrelated (sapi/fpm/tests/reload-uses-sigkill-as-last-measure.phpt)

nikic
nikic approved these changes Jan 7, 2020
View reviewed changes
Copy link
Member

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Whew, that was more tricky than I expected :)

ext/mbstring/libmbfl/mbfl/mbfilter.c
Comment on lines +881 to +883
if (offset < 0) {
result = haystack_length + offset;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable.

@php-pulls php-pulls closed this in 483efc7 Jan 7, 2020
@Girgias Girgias deleted the allow-empty-needle-mbstring branch January 7, 2020 21:55
@boxcleverliam
Copy link

Sorry if this is a newbie question! Is there a bug or an RFC that can be referenced to say why this change was made?

@Girgias
Copy link
Member Author

Girgias commented Apr 12, 2024

Sorry if this is a newbie question! Is there a bug or an RFC that can be referenced to say why this change was made?

It was made to be consistent with the non-mbstring version of the function

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
Waiting on Author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Girgias @nikic @cmb69 @boxcleverliam