Fix GH-10627: mb_convert_encoding crashes PHP on Windows #10628
Conversation
Fixes phpGH-10627 The php_mb_convert_encoding() function can return NULL on error, but this case was not handled, which led to a NULL pointer dereference and hence a crash.
| var_dump(mb_convert_encoding($data, 'UTF-8', 'auto')); | ||
| $data = [$str => 'abc', 'abc' => 'def']; | ||
| var_dump(mb_convert_encoding($data, 'UTF-8', 'auto')); | ||
|
|
There was a problem hiding this comment.
I reproduced --enable-debug on WSL on Ubuntu and it is seems C NULL problem. But 3v4l looks works fine and result is wrong because maybe without --enable-debug option.
@alexdowad what do you think?
There was a problem hiding this comment.
Hmm. That is interesting. I'm not sure of the reason right now.
This bug occurs if 'auto' is passed as the desired encoding, and mbstring is not able to detect the encoding of the string.
I might need to check under which circumstances that can happen. In any case, this bug fix is definitely needed.
|
@nielsdos This looks good. Thanks very much. This patch will definitely not apply cleanly on It will be appreciated if, when fixing the merge conflicts, you make sure that all calls to |
|
@alexdowad Can you check that the behaviour on master makes sense? As mbstring seems to be able to decode this now without any issues and not sure if this is meant to be the right behaviour or not. |
|
Dear @Girgias, I did look at the code on If @nielsdos wants to merge into |
|
@alexdowad Thank you for the review. |
|
Ah, sorry, I see. You already merged down into First thing I notice is that @MaxKellermann's recent fix was wiped out during the merge. |
|
I'm not sure who the "you" refers to. Just to be clear: I did not merge it. 😅 |
|
"You" is @Girgias in this case. |
Oh FFS, I didn't triple check everything as I was already working through the merge conflict with that... |
|
@Girgias I think the test case also doesn't actually exercise the code which it is intended to exercise on The NULL checks which you added are needed if 1) Hmm... I really wonder why that test case 'tripped' the bug even on PHP 8.1... there might be something else going on there which I should look into. It doesn't look to me right now like it should. But definitely the bug was a real bug. To exercise the bug fix, we need to provide strings which are not valid in any of the provided text encodings (and there needs to be more than one). Using a string like |
|
As for behavior of the fix on What this means is that if someone passes an array to This seems like a reasonable choice to me, but we should try to get it documented in the PHP manual at some point. |
|
You're the encoding and MBstring expert here, so I'll let you deep dive to figure what's going on :D I can always review whatever PR you come up with. RE: Docs: I think the mbstring docs probably need an overhaul and documenting those quirks makes sense, although somewhat suboptimal. |
|
@Girgias Fair enough, I'll submit some improvements soon. |
|
Well, I was wrong about @MaxKellerman's fix getting wiped out by the merge. It's just fine. |
Fixes GH-10627
The php_mb_convert_encoding() function can return NULL on error, but this case was not handled, which led to a NULL pointer dereference and hence a crash.