Skip to content
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.

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

Fix various base64_decode bugs. #1923

Closed
wants to merge 6 commits into from

Conversation

3 participants
@Metabolix
Copy link
Contributor

Metabolix commented May 25, 2016

Fix base64_decode bugs 72264, 72263, 72152, with tests.

This is for PHP-7.0, because the patches don't cleanly apply to PHP-5.6 (which uses char* instead of zend_string).

Metabolix added some commits May 25, 2016

base64_decode: remove redundant check
If length == 0 || *current != '=' is false, the for loop will always
end up in this same point, until the if statement becomes true.
Thus, the if statement is not needed.
base64_decode: fix bug #72152 (fail on NUL bytes in strict mode)
This added check is actually for NOT failing in NON-strict mode.
The ch == -2 check later causes the desired failure in strict mode.
base64_decode: remove redundant code
case 1 is already handled in the first lines of the for loop;
it would only be entered in the invalid case where the string
continues past the defined length (ch != 0 but length-- == 0).

case 2 and case 3 are redundant, since k >= j and later the
string is truncated to j characters anyway.

@laruence laruence added the Bugfix label May 28, 2016

@Metabolix

This comment has been minimized.

Copy link
Contributor Author

Metabolix commented Jun 29, 2016

Any comments on this? Is there something wrong with the patches?

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 1, 2016

I don't think anything's wrong. I'll try to land this on the next week.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 5, 2016

Merged into 7.0 and upwards. Thanks for your thorough work on this!

I think that the FIXME "why do we still allow invalid padding in other places in the middle of the string?" at least should be resolved as well. Padding in the middle of the string really makes no sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.