Don't assume that file binary exists on *nix OS #26886

teohhanhui · 2018-04-11T12:55:36Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

Certain lightweight distributions such as Alpine Linux (popular for smaller Docker images) do not include it by default.

nicolas-grekas · 2018-04-12T12:47:32Z

src/Symfony/Component/HttpFoundation/File/MimeType/FileBinaryMimeTypeGuesser.php

@@ -43,7 +43,7 @@ public function __construct($cmd = 'file -b --mime %s 2>/dev/null')
     */
    public static function isSupported()
    {
-        return '\\' !== DIRECTORY_SEPARATOR && function_exists('passthru') && function_exists('escapeshellarg');
+        return '\\' !== DIRECTORY_SEPARATOR && function_exists('passthru') && function_exists('escapeshellarg') && null !== shell_exec('command -v file');


since the performance penalty for doing this call is high, the result should be cached

stof · 2018-04-12T13:57:23Z

src/Symfony/Component/HttpFoundation/File/MimeType/FileBinaryMimeTypeGuesser.php

@@ -43,7 +43,7 @@ public function __construct($cmd = 'file -b --mime %s 2>/dev/null')
     */
    public static function isSupported()
    {
-        return '\\' !== DIRECTORY_SEPARATOR && function_exists('passthru') && function_exists('escapeshellarg');
+        return '\\' !== DIRECTORY_SEPARATOR && function_exists('passthru') && function_exists('escapeshellarg') && static::hasFileBinary();


this must be self:: to access a private API. Otherwise, a child class would break when calling this code (as it would try to call ChildClass::hasFileBinary)

I've just tried this out. It'd only break when ChildClass overrides hasFileBinary. But yes, there is no reason for us to use static:: here, since it's a private method anyway.

we could cache the full expression, including the function_exists check (and thus not need any new method btw)

Done and done.

stof · 2018-04-12T13:58:24Z

src/Symfony/Component/HttpFoundation/File/MimeType/FileBinaryMimeTypeGuesser.php

+    {
+        static $exists;
+
+        return isset($exists) ? $exists : ($exists = null !== shell_exec('command -v file'));


this should use passthru as well to run the command. Otherwise, it requires adding shell_exec to the list of existing functions (as these are functions which are likely to get disabled on some hosting)

teohhanhui · 2018-04-12T14:51:52Z

Status: Needs Review

nicolas-grekas · 2018-04-12T18:07:01Z

src/Symfony/Component/HttpFoundation/File/MimeType/FileBinaryMimeTypeGuesser.php

@@ -43,7 +43,33 @@ public function __construct($cmd = 'file -b --mime %s 2>/dev/null')
     */
    public static function isSupported()
    {
-        return '\\' !== DIRECTORY_SEPARATOR && function_exists('passthru') && function_exists('escapeshellarg');
+        static $supported;


If you don't mind, here is an equivalent shorter implementation:

+ static $supported = null; + + if (null !== $supported) { + return $supported; + } + + if ('\\' === DIRECTORY_SEPARATOR || !function_exists('passthru') || !function_exists('escapeshellarg')) { + return $supported = false; + } + + ob_start(); + passthru('command -v file', $return); + $binPath = trim(ob_get_clean()); + + return $supported = 0 === $return && preg_match('#^/[^\0/]#', $binPath);

But not as readable...

Less cyclomatic complexity so easier to understand actually to me...

Cyclomatic complexity might not be a good measure for readability: https://web.eecs.umich.edu/~weimerw/p/weimer-tse2010-readability-preprint.pdf

nicolas-grekas · 2018-04-12T18:07:57Z

src/Symfony/Component/HttpFoundation/File/MimeType/FileBinaryMimeTypeGuesser.php

+
+        $binPath = trim(ob_get_clean());
+
+        if (!preg_match('#^(?:/[^\0/])+#', $binPath)) {


why this specific regex? can the NUL char actually happen?
also, it looks like this could be just '#^/[^\0/]#'

Yes, it's possible to have a NUL character:

ob_start(); passthru('find -maxdepth 1 -print0'); $output = trim(ob_get_clean()); echo ord($output[1]);

gives:

0

nicolas-grekas · 2018-04-13T12:12:37Z

Yes, it's possible to have a NUL character

But your example doesn't use command -v, so I'm not sure it applies, does it?

About the proposal I made, I did it because I found it harder to review when ob_* closing functions are called in two different code paths. I made the proposal to make Symfony better, not to bother you.

teohhanhui · 2018-04-13T16:54:58Z

But your example doesn't use command -v, so I'm not sure it applies, does it?

Rather not assume that command -v will return what we expect it to return. It doesn't complicate the regex much anyway...

About the proposal I made, I did it because I found it harder to review when ob_* closing functions are called in two different code paths. I made the proposal to make Symfony better, not to bother you.

Got it. I will make the change.

teohhanhui · 2018-04-13T17:10:09Z

Removed the preg_match as it's just unnecessary (and what if file is a shell built-in lol)

fabpot · 2018-04-17T04:49:22Z

src/Symfony/Component/HttpFoundation/File/MimeType/FileBinaryMimeTypeGuesser.php

-        return '\\' !== DIRECTORY_SEPARATOR && function_exists('passthru') && function_exists('escapeshellarg');
+        static $supported;
+
+        if (isset($supported)) {


I would use if (null !== $supported) here instead.

Certain lightweight distributions such as Alpine Linux (popular for smaller Docker images) do not include it by default.

teohhanhui · 2018-04-17T09:16:44Z

Status: Needs Review

fabpot · 2018-04-17T10:07:11Z

Thank you @teohhanhui.

This PR was merged into the 2.7 branch. Discussion ---------- Don't assume that file binary exists on *nix OS | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Certain lightweight distributions such as Alpine Linux (popular for smaller Docker images) do not include it by default. Commits ------- e2c1f24 Don't assume that file binary exists on *nix OS

…ialization (nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [HttpFoundation] Fix perf issue during MimeTypeGuesser intialization | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27307 | License | MIT | Doc PR | - introduced in #26886 ![image](https://user-images.githubusercontent.com/243674/40451947-918f5358-5ee0-11e8-9f1a-cf707bf3cefa.png) Commits ------- f8e7a18 [HttpFoundation] Fix perf issue during MimeTypeGuesser intialization

carsonbot added Status: Needs Review Bug labels Apr 11, 2018

nicolas-grekas added this to the 2.7 milestone Apr 12, 2018

nicolas-grekas reviewed Apr 12, 2018

View reviewed changes

xabbuh added the HttpFoundation label Apr 12, 2018

teohhanhui force-pushed the patch-3 branch 2 times, most recently from 825f359 to 8b1c680 Compare April 12, 2018 13:38

stof requested changes Apr 12, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 12, 2018

teohhanhui force-pushed the patch-3 branch 2 times, most recently from ad148d7 to 526f888 Compare April 12, 2018 14:44

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 12, 2018

nicolas-grekas reviewed Apr 12, 2018

View reviewed changes

teohhanhui force-pushed the patch-3 branch from 526f888 to 88b9434 Compare April 12, 2018 19:32

teohhanhui force-pushed the patch-3 branch from 88b9434 to ed391df Compare April 13, 2018 17:09

nicolas-grekas approved these changes Apr 13, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 13, 2018

fabpot requested changes Apr 17, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Apr 17, 2018

Don't assume that file binary exists on *nix OS

e2c1f24

Certain lightweight distributions such as Alpine Linux (popular for smaller Docker images) do not include it by default.

teohhanhui force-pushed the patch-3 branch from ed391df to e2c1f24 Compare April 17, 2018 09:15

carsonbot added Status: Needs Review Status: Reviewed and removed Status: Needs Work Status: Needs Review labels Apr 17, 2018

fabpot approved these changes Apr 17, 2018

View reviewed changes

fabpot merged commit e2c1f24 into symfony:2.7 Apr 17, 2018

This was referenced Apr 27, 2018

Release v2.7.46 #27070

Merged

Release v2.8.39 #27095

Merged

Release v3.4.9 #27102

Merged

Release v4.0.9 #27103

Merged

nicolas-grekas mentioned this pull request May 23, 2018

[HttpFoundation] Fix perf issue during MimeTypeGuesser intialization #27359

Merged

teohhanhui deleted the patch-3 branch March 12, 2019 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't assume that file binary exists on *nix OS #26886

Don't assume that file binary exists on *nix OS #26886

teohhanhui commented Apr 11, 2018

nicolas-grekas Apr 12, 2018

teohhanhui Apr 12, 2018

stof Apr 12, 2018

teohhanhui Apr 12, 2018

nicolas-grekas Apr 12, 2018

teohhanhui Apr 12, 2018

stof Apr 12, 2018

teohhanhui Apr 12, 2018

teohhanhui commented Apr 12, 2018

nicolas-grekas Apr 12, 2018

teohhanhui Apr 12, 2018

nicolas-grekas Apr 12, 2018

teohhanhui Apr 13, 2018

nicolas-grekas Apr 12, 2018

teohhanhui Apr 12, 2018

nicolas-grekas commented Apr 13, 2018 •

edited

Loading

teohhanhui commented Apr 13, 2018

teohhanhui commented Apr 13, 2018

fabpot Apr 17, 2018

teohhanhui commented Apr 17, 2018

fabpot commented Apr 17, 2018


		$binPath = trim(ob_get_clean());

		if (!preg_match('#^(?:/[^\0/])+#', $binPath)) {

Don't assume that file binary exists on *nix OS #26886

Don't assume that file binary exists on *nix OS #26886

Conversation

teohhanhui commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teohhanhui commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 13, 2018 • edited Loading

teohhanhui commented Apr 13, 2018

teohhanhui commented Apr 13, 2018

Choose a reason for hiding this comment

teohhanhui commented Apr 17, 2018

fabpot commented Apr 17, 2018

nicolas-grekas commented Apr 13, 2018 •

edited

Loading