Be consistent in clearing out in php_iconv_string() #3037

pprindeville · 2018-01-25T01:54:53Z

Tested by @MikePetullo here

tpunt · 2018-01-25T10:55:53Z

This looks good to me. Also, whilst this is a little tangential, the calling sites for php_iconv_string could also be cleaned up a little by not even bothering to free the allocated out buffer, since it will always be freed by the function itself on error (so places such as here and here).

pprindeville · 2018-01-25T18:35:08Z

@tpunt Did you want me to make that PR as well? Or amend this one?

Depending on which version of libiconv you're using, php_iconv_string() doesn't always null out *out as part of its initialization. This patch makes that behavior invariant. Submitted upstream as php/php-src#3037 where it's approved and waiting a merge. Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>

tpunt · 2018-01-25T18:59:14Z

Since removing the superfluous buffer freeing logic would also fix this problem, I'd say just amend this PR with those changes too.

pprindeville · 2018-01-25T19:50:21Z

Done. Ran make test with no surprises.

Depending on which version of libiconv you're using, php_iconv_string() doesn't always null out *out as part of its initialization. This patch makes that behavior invariant. Submitted upstream as php/php-src#3037 where it's approved and waiting a merge. Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>

pprindeville · 2018-01-25T20:29:45Z

@tpunt Are these being affected as a side-effect?

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Bug #48147 (iconv with //IGNORE cuts the string) [ext/iconv/tests/bug48147.phpt]
Bug #52211 (iconv() returns part of string on error) [ext/iconv/tests/bug52211.phpt]
Bug #74230 iconv fails to fail on surrogates [ext/intl/tests/bug74230.phpt]
=====================================================================

Haven't read the test cases so I'm not sure what the scenarios are...

tpunt · 2018-01-25T20:47:30Z

Ah, my mistake, it appears the out buffer is not always being freed when erroring. It's strange that the function frees the buffer in the first ifdefed block, but not in the second... Whilst it would probably be best to make them the same (always free on error, since it allocates the buffer), I think that's outside the scope of this PR. The original fix of just nullifying *out therefore seems best.

Also, don't bother checking returned point in error case since it will always be NULL (and not require free()ing, obviously).

pprindeville · 2018-01-25T21:19:18Z

Just trying to see what Travis finds that doesn't turn up locally if we do as you suggest and always free on error.

pprindeville · 2018-01-25T21:49:12Z

@tpunt Well, I took your suggestion all the way and the build didn't have any Failed Tests.

I separated the commits to the original/minimal fix plus the more complete fixes. You can merge just the first or both, as you see fit.

Depending on which version of libiconv you're using, php_iconv_string() doesn't always null out *out as part of its initialization. This patch makes that behavior invariant. Submitted upstream as php/php-src#3037 where it's approved and waiting a merge. Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>

pprindeville · 2018-01-26T18:56:20Z

@tpunt Let me know if you need anything else, otherwise I'm assuming we're good to go.

tpunt · 2018-01-26T19:21:12Z

@pprindeville This looks fine to me. I don't have the appropriate karma to merge your PR, though. Perhaps @nikic could merge this instead.

pprindeville · 2018-01-26T19:22:20Z

@tpunt Thanks, understood.

nikic · 2018-01-26T20:21:41Z

ext/iconv/iconv.c

 	iconv_close(cd);

 	if (result == (size_t)(-1)) {
+		zend_string_free(out_buf);


This will cause a use-after-free of out_buf in the code below. If we free out_buf here, then the breaks in the switch below need to be changed to returns. (As is the case for PHP_ICONV_ERR_UNKNOWN, which was the only freeing case previously.)

Okay, since it's not clear what the semantic is for "return an error but leave a partially populated buffer" I suggest just going with the first commit.

nikic · 2018-01-26T20:25:09Z

There is one use of php_iconv_string in https://github.com/pprindeville/php-src/blob/3763c8f1645983b5abc37c60597e1ecc1bf89019/ext/iconv/iconv.c#L437 where the output is used without an error check. I'm thinking that maybe this is why the code made a distinction between different error types -- it want's this case to be able to use the output even if there was some non-fatal error?

No idea whether or not that's really the case, or if this is just an oversight.

nikic · 2018-01-26T20:33:31Z

I'd say merge the *out = NULL part for 7.1+ and the rest only for master.

pprindeville · 2018-01-26T20:38:51Z

I'd say merge the *out = NULL part for 7.1+ and the rest only for master.

Given your previous comment, maybe just go with the first commit everywhere?

pprindeville · 2018-01-26T20:39:31Z

Should I prep separate PR's for 7.1 and 7.2 or will someone else cherry-pick them?

nikic · 2018-01-26T21:30:49Z

Okay, let's go with the conservative variant... I've merged the first commit as aad76a9 into 7.1+.

pprindeville · 2018-01-26T23:11:16Z

@nikic Do I need to prepare PR's for 7.2 and master or is that already done?

nikic · 2018-01-26T23:20:49Z

@pprindeville This is already done. We always apply changes to the lowest branch and then merge upward.

pprindeville · 2018-01-26T23:39:57Z

Okay, thanks.

cmb69 · 2018-02-24T22:40:38Z

JFTR: this fixed bug 75867.

Depending on which version of libiconv you're using, php_iconv_string() doesn't always null out *out as part of its initialization. This patch makes that behavior invariant. Submitted upstream as php/php-src#3037 where it's approved and waiting a merge. Signed-off-by: Philip Prindeville <philipp@redfish-solutions.com>

tpunt approved these changes Jan 25, 2018

View reviewed changes

pprindeville force-pushed the bug/75867 branch from 7d3303a to 8b6882d Compare January 25, 2018 19:48

Be consistent in clearing out in php_iconv_string()

31e53f0

Also, don't bother checking returned point in error case since it will always be NULL (and not require free()ing, obviously).

pprindeville force-pushed the bug/75867 branch from 8b6882d to 31e53f0 Compare January 25, 2018 21:17

Always free out_buf in php_iconv_string()

3763c8f

nikic reviewed Jan 26, 2018

View reviewed changes

nikic closed this Jan 26, 2018

Be consistent in clearing out in php_iconv_string() #3037

Be consistent in clearing out in php_iconv_string() #3037

Uh oh!

Conversation

pprindeville commented Jan 25, 2018

Uh oh!

tpunt commented Jan 25, 2018

Uh oh!

pprindeville commented Jan 25, 2018

Uh oh!

tpunt commented Jan 25, 2018

Uh oh!

pprindeville commented Jan 25, 2018

Uh oh!

pprindeville commented Jan 25, 2018

Uh oh!

tpunt commented Jan 25, 2018

Uh oh!

pprindeville commented Jan 25, 2018

Uh oh!

pprindeville commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pprindeville commented Jan 26, 2018

Uh oh!

tpunt commented Jan 26, 2018

Uh oh!

pprindeville commented Jan 26, 2018

Uh oh!

nikic Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

pprindeville Jan 26, 2018

Choose a reason for hiding this comment

Uh oh!

nikic commented Jan 26, 2018

Uh oh!

nikic commented Jan 26, 2018

Uh oh!

pprindeville commented Jan 26, 2018

Uh oh!

pprindeville commented Jan 26, 2018

Uh oh!

nikic commented Jan 26, 2018

Uh oh!

pprindeville commented Jan 26, 2018

Uh oh!

nikic commented Jan 26, 2018

Uh oh!

pprindeville commented Jan 26, 2018

Uh oh!

cmb69 commented Feb 24, 2018

Uh oh!

Uh oh!

pprindeville commented Jan 25, 2018 •

edited

Loading