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 missing setlocale with php 8 #29695

Merged
merged 6 commits into from
Nov 16, 2021

Conversation

YoitoFes
Copy link
Contributor

@YoitoFes YoitoFes commented Nov 13, 2021

When php version = 8, basename('§') returns '§' even if LC_ALL is non-UTF-8 locale.
Since OC_Util::isSetLocaleWorking() assumes basename('§') returns empty string with non-UTF-8 locale,
the function with php 8 incorrectly skips setlocale("C.UTF-8").

Fix it by using escapeshellcmd instead of basename.

This issue found in #29296 and The PR may be able to resolve it.

@szaimen szaimen added this to the Nextcloud 24 milestone Nov 13, 2021
@szaimen szaimen linked an issue Nov 13, 2021 that may be closed by this pull request
@szaimen szaimen added 3. to review Waiting for reviews bug labels Nov 13, 2021
@szaimen szaimen requested review from ChristophWurst, a team, PVince81, juliusknorr and skjnldsv and removed request for a team November 13, 2021 18:22
When php version = 8, basename('§') does not bug even if LC_ALL is non-UTF-8 locale.
This cause OC_Util::isSetLocaleWorking() to skip setlocale("C.UTF-8").

Fix it by using escapeshellcmd instead of basename.

Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>
Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>
@YoitoFes YoitoFes force-pushed the fix_missing_setlocale branch from 73f133f to d2eb5aa Compare November 14, 2021 00:20
@YoitoFes YoitoFes closed this Nov 14, 2021
@YoitoFes YoitoFes reopened this Nov 14, 2021
@PVince81 PVince81 requested a review from come-nc November 15, 2021 09:20
Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 15, 2021
@PVince81
Copy link
Member

/backport to stable23

@solracsf
Copy link
Member

What if escapeshellcmd() is a disabled function in php? 🤔

@PVince81
Copy link
Member

What if escapeshellcmd() is a disabled function in php? thinking

not sure if it's as common as disabling the exec and shell_exec

but you're right to be careful, is there a way to detect disabled functions ?
if disabled then we could fall back to basename

@solracsf
Copy link
Member

@PVince81 https://www.php.net/manual/en/function.function-exists.php

if (!function_exists('escapeshellcmd')) {
...
}

@YoitoFes
Copy link
Contributor Author

Is escapeshellarg disabled less often than escapeshellcmd? escapeshellarg is used in Nextcloud's source code while escapeshellcmd is not.

@solracsf
Copy link
Member

Hard to say, but it is always a good practice check if the function exists before call it, and have a fallback 😉

Using escapeshellcmd to get current locale causes error
if the function is disabled.

Add fallbacks to prevent the error.

Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>
Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>
@YoitoFes
Copy link
Contributor Author

YoitoFes commented Nov 15, 2021

I think basename can not be used as a fallback since the function with php8 causes the issue.
So I added escapeshellarg and preg_match as fallbacks.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Fine by me 👍

Signed-off-by: Naoto Kobayashi <naoto.kobayashi4c@gmail.com>
@PVince81 PVince81 merged commit 67ff36b into nextcloud:master Nov 16, 2021
@welcome
Copy link

welcome bot commented Nov 16, 2021

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@PVince81
Copy link
Member

seems the bot is lagging, no further backports yet?

@PVince81
Copy link
Member

/backport to stable22

@PVince81
Copy link
Member

/backport to stable21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No video preview when the folder has UTF8 characters
6 participants