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

php7+ crash on windows 1803 strftime format %Z #4752

Merged
merged 4 commits into from May 14, 2018

Conversation

Projects
None yet
5 participants
@albertlast
Collaborator

albertlast commented May 6, 2018

I notice on my env that apache crash since windows 10 1803

Test code for this issue:

<?php
$value = strftime('%Z');
echo $value.'%Z';
echo '<br>';

setlocale(LC_TIME,'en_US.UTF-8');
$value = strftime('%Z');
echo $value.'%Z';
echo '<br>';

setlocale(LC_CTYPE,'en_US.UTF-8');
$value = strftime('%Z');
echo $value.'%Z';
echo '<br>';
php crash on windows 1803 strftime format %Z
Signed-off-by: albertlast albertlast@hotmail.de

@albertlast albertlast changed the title from php crash on windows 1803 strftime format %Z to php7+ crash on windows 1803 strftime format %Z May 6, 2018

@jdarwood007

This comment has been minimized.

Member

jdarwood007 commented May 6, 2018

Did you file a bug report with PHP as well? Seems relevant to them to patch this as well.

@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 6, 2018

nope

@wintstar

This comment has been minimized.

wintstar commented May 6, 2018

I had the same problem. Your fix works for me.

@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 6, 2018

Since i got the confirmation from wintstar i open a bug report on php

@jdarwood007

This comment has been minimized.

Member

jdarwood007 commented May 6, 2018

Awesome. If we can get confirmed affected versions and if they are mainstream in distros, I can see fixing this. If its not a release making distros, then it may not be worth it.
I.e, if you downloaded something like wamp or iis config tool.

@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 6, 2018

What is not worth?

@MissAllSunday

This comment has been minimized.

Contributor

MissAllSunday commented May 8, 2018

Fix the issue on SMF side. He means to wait until we know which versions are affected by this issue and patch it up if popular distros ship those versions.

Although I could argue (without having exact figures of course) that a good chunk of mod authors do use tools like xampp or wamp in windows to develop and that a good amount of those users are also using win10 so it might be worth to fix even if this doesn't affect any other major distributions.

@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 8, 2018

When you read the documention on php side,
you notice that %z and %Z are not supported anyway on windows.
So it make less sense to test this.

@wintstar

This comment has been minimized.

wintstar commented May 8, 2018

You might also consider that this bug could affect hosters with Windows Server. Or not?

@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 8, 2018

@Sesquipedalian @colinschoen would be nice if we get the fix asap,
the development without this fix is very hard...

add a more generic approache and added z to the list
Signed-off-by: albertlast albertlast@hotmail.de
@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 13, 2018

Add more generic way and set z to the list of unsupportedFormats

@jdarwood007

This comment has been minimized.

Member

jdarwood007 commented May 13, 2018

If Z isn't supported on windows, I'm fine with removing it if the server is windows based. Although I don't see that mentioned in the php.net/date at all.

@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 13, 2018

@Sesquipedalian

This comment has been minimized.

Member

Sesquipedalian commented May 13, 2018

I think you could accomplish the same goal more simply by adding || ($context['server']['is_windows'] && in_array($format, array('z', 'Z'))) to the end of this if statement.

@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 14, 2018

Nope this point is to late.
The logic needs to be in front of: $value = @strftime('%' . $format);

@Sesquipedalian

This comment has been minimized.

Member

Sesquipedalian commented May 14, 2018

In that case, I would insert the following bit of code into here immediately before the $value = @strftime('%' . $format); line.

// Avoid a crashing bug with PHP 7 on certain versions of Windows
if ($context['server']['is_windows'] && in_array($format, array('z', 'Z')))
{
	$unsupportedFormats[] = $format;
	continue;
}

albertlast added some commits May 14, 2018

more compact way
Signed-off-by: albertlast albertlast@hotmail.de
@albertlast

This comment has been minimized.

Collaborator

albertlast commented May 14, 2018

Done

@@ -824,6 +826,13 @@ function timeformat($log_time, $show_today = true, $offset_type = false, $proces
{
foreach($strftimeFormatSubstitutions as $format => $substitution)
{
// Avoid a crashing bug with PHP 7 on certain versions of Windows
if ($context['server']['is_windows'] && in_array($format, $unsupportedFormatsWindows))

This comment has been minimized.

@Sesquipedalian

Sesquipedalian May 14, 2018

Member

Should be in_array($format, array('z', 'Z'))

This comment has been minimized.

@albertlast

albertlast May 14, 2018

Collaborator

In my eyes is worst to read and hard to maintaine,
reasone why i keep to define the array in front of the function.

This comment has been minimized.

@Sesquipedalian

Sesquipedalian May 14, 2018

Member

Further evidence that I should go to bed now.

@Sesquipedalian Sesquipedalian merged commit ec5bbc5 into SimpleMachines:release-2.1 May 14, 2018

2 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment