Fix bug #74866 #2612

flaupretre · 2017-07-07T00:44:09Z

Work in progress

weltling

IMO the usage of DL_LOAD without the previous stat is a better approach. There are cases, where the shared obj doesn't have to be available under the exact path in CWD.

What I was wondering yesterday checking the previous patch, is that there was a code duplication also going into php_ini.c. Probably it could be avoided by grouping the code into reusable functions for better maintanance. Fe I'd see at least these pieces that can be grouped

build ext basename from ext name
check whether the given filename looks like ext name
build the extension path
load an arbitrary shared lib
OFC the grouping could be done, after the right fix is in place, too.

Thanks.

weltling · 2017-07-07T12:18:49Z

ext/standard/dl.c

 		return FAILURE; /* Not full path given or extension_dir is not set */
 	}

-	/* load dynamic symbol */


Better not to duplicate code, but get it into a function.

weltling · 2017-07-07T12:55:35Z

ext/standard/dl.c

+			err1 = estrdup(err);
+			GET_DL_ERROR(); /* free the buffer storing the error */
+#endif



After thinking about it, might it make sense to still check whether filename looks like an extension name? Fe like strcmp against PHP_SHLIB_EXT_PREFIX and PHP_SHLIB_SUFFIX. Reason - if it's already an ext filename, it won't produce a weird output like php_php_bz2.dll.dll or bz2.so.so and an error can be signaled immediately. Again, could be still packed into a simple function to use on teh Zend side as well.

weltling · 2017-07-07T13:17:00Z

Oh, looks like I was commenting on an outdated version.

flaupretre · 2017-07-07T13:40:06Z

@weltling I just pushed a new version using a new php_attempt_dl_load() function, avoiding duplicates.

Can you test this version ? It should fix the issue for PHP extensions.

I prefer not to analyze the input arg, trying to determine if it is a filename or an extension name. Issuing an extensive error message, listing every path we tried to load, and the corresponding error messages is clear enough, IMO.

I agree it may lead to weird error messages like : 'Unable to load dynamic library 'php_bz2.dll' (tried : c:\php\ext\php_bz2.dll -> No such file or directory, c:\php\ext\php_php_bz2.dll.dll -> No such file or directory)' but the message is easy to understand from the user's POV.

weltling · 2017-07-07T13:45:52Z

@flaupretre what about the Zend ext part? Seems it has same issue. Otherwise can test right now.

Thanks

weltling · 2017-07-07T14:45:37Z

ext/standard/dl.c

+
+static void *php_attempt_dl_load(char *path, char **errp)
+{
+	char *handle;


void *handle;

Fixed. Thanks.

weltling · 2017-07-07T14:45:45Z

ext/standard/dl.c

+		(*errp) = estrdup(err);
+		GET_DL_ERROR(); /* free the buffer storing the error */
+#endif
+	}


return NULL;

Fixed. Thanks.

weltling · 2017-07-07T14:51:42Z

Yeah, this approach could work. The Zend ext part isn't touched yet though. When it's done, i can make a test build for the reporter to test. Need to check same on Linux at least.

Thanks.

weltling · 2017-07-07T14:53:52Z

About the error message - as for me, it simply look weird. If a name looking like someext.so were passed, it's already an ext name. Well, theoretically someone could name an ext hello.so.so, dunno. IMO no need so support such weirdness.

Thanks.

flaupretre · 2017-07-07T15:27:24Z

I agree it looks weird but analyzing the name would make the logic much more complex. Remember that we'll end up displaying weird names ONLY when nothing can be loaded. And, with the time, as people will use extension names more and more often, weird messages will slowly disappear...

weltling · 2017-07-07T16:23:37Z

Yeah, OFC handling the err message is not necessary for the bug fix. I'll check with RMs yet, so at least they're aware about this nuance.

Ok, so waiting for the Zend part yet, will test and produce a build then.

Thanks.

flaupretre · 2017-07-07T17:00:29Z

@weltling Just pushed the fix for PHP and Zend extensions. Removed every stat() call, using DL_LOAD() only everywhere.

A small detail : test for bug #71273 (tests/basic/bug71273.phpt) failed during appveyor tests. This must be linked to the error message having been modified but I don't see why the pattern does not match. It works on Linux. Unfortunately, as I cannot test the new version on Windows, I cannot see the command output. The command it execs is :

"php -n -d html_errors=on -d extension_dir=a/é/w -d extension=php_kartoffelbrei.dll -v 2>&1"

If you can check that this test now fails on Windows, run the command, and send me the output, it would help a lot. Thanks.

weltling · 2017-07-07T18:33:08Z

main/php_ini.c

 }
 /* }}} */

+static void *attempt_dl_load(char *path, char **errp)


Why not just export the other one through dl.h?

I wanted to avoid calling ext/standard from 'main'. As I didn't know if those parts could communicate with eaach other, I did two identical static functions. If a function in ext/standard can be loaded from main, I'll do it.

That's fine, ext/standard cannot be disabled. By that - such a usage is legit, and in fact, there are quite a few of other places doing it in main.

weltling · 2017-07-07T18:36:02Z

main/php_ini.c

 #include "win32/php_registry.h"
+#include "win32/param.h"
+#include "win32/winutil.h"
+#define GET_DL_ERROR()	php_win_err()


These macros should be exported through the dl.h as well.

weltling · 2017-07-07T18:51:38Z

@flaupretre great, except a few comments seems it's close to finish now. I'd suggest to export that function like php_attempt_dl_load -> php_load_shlib. DL is about extensions, here we just load an arbitrary shared lib.

The failing test in the earlier AppVeyor run was most likely due to the missing return NULL, etc. Visual Studio is a strict compiler, such bugs come out quite quick. The latest CI runs are all green. Local build seems ok so far, too.

Need to check the Linux part at least yet.

Thanks.

… Zend extensions

flaupretre · 2017-07-07T23:10:16Z

@weltling Just pushed a new version. Hope it matches your suggestions. I created a PHPAPI php_load_shlib() function in ext/standard/dl.c and use it for both PHP and Zend extensions.

flaupretre · 2017-07-07T23:11:56Z

I probably won't have a Linux host available for testing this week-end. I'll do it monday morning.

taozywu · 2017-07-08T02:49:35Z

ext/standard/dl.c

 	zend_module_entry *(*get_module)(void);
 	int error_type, slash_suffix;
-	char *extension_dir;
+	char *extension_dir, *err1, *err2;


you can define 'err1' or 'err2', use 'err_one', 'err_two' instead!
What do you think of it?

weltling · 2017-07-08T21:08:15Z

@flaupretre I've just tested on Linux, at first glance looks good. I've also linked a Windows snapshot build with this patch. Waiting for yours and reporters tests yet to proceed further.

Thanks.

weltling · 2017-07-10T20:03:33Z

@flaupretre any news on your side?

Thanks.

flaupretre · 2017-07-10T23:32:15Z

@weltling Didn't have time for that today. Will test on Linux tomorrow.

weltling · 2017-07-13T12:39:16Z

I just applied this in master as 0782a7f. @flaupretre additional verification would still be great. /cc @remicollet

Thanks.

flaupretre · 2017-07-14T16:23:58Z

@weltling @remicollet Just checked on Linux. Loading PHP and Zend extensions, nominal case and error. Everything looks OK.

weltling · 2017-07-14T16:25:21Z

Thanks! And great holiday yet :)

flaupretre force-pushed the fix_74866 branch from 4fc719c to d2ea6c0 Compare July 7, 2017 01:01

krakjoe added Waiting on Author Bug labels Jul 7, 2017

Fix #74866 for PHP extensions

ef0325b

flaupretre force-pushed the fix_74866 branch from d2ea6c0 to ef0325b Compare July 7, 2017 12:55

weltling suggested changes Jul 7, 2017

View reviewed changes

Change error message

cd0751c

flaupretre force-pushed the fix_74866 branch from 9cbe079 to cd0751c Compare July 7, 2017 13:56

weltling reviewed Jul 7, 2017

View reviewed changes

flaupretre force-pushed the fix_74866 branch from 58f5e34 to 1dcfc5e Compare July 7, 2017 15:20

flaupretre added 2 commits July 7, 2017 18:43

Fix typos

1438662

Fix #74866 for Zend extensions

a571016

flaupretre force-pushed the fix_74866 branch from 1dcfc5e to a571016 Compare July 7, 2017 16:46

weltling reviewed Jul 7, 2017

View reviewed changes

Create exported php_load_shlib() function and use it for both PHP and…

068bc08

… Zend extensions

taozywu reviewed Jul 8, 2017

View reviewed changes

weltling closed this Jul 13, 2017

flaupretre deleted the fix_74866 branch July 14, 2017 16:25

Fix bug #74866 #2612

Fix bug #74866 #2612

Uh oh!

Conversation

flaupretre commented Jul 7, 2017

Uh oh!

weltling left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltling commented Jul 7, 2017

Uh oh!

flaupretre commented Jul 7, 2017

Uh oh!

weltling commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltling commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weltling commented Jul 7, 2017

Uh oh!

flaupretre commented Jul 7, 2017

Uh oh!

weltling commented Jul 7, 2017

Uh oh!

flaupretre commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltling commented Jul 7, 2017

Uh oh!

flaupretre commented Jul 7, 2017

Uh oh!

flaupretre commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weltling commented Jul 8, 2017

Uh oh!

weltling commented Jul 10, 2017

Uh oh!

flaupretre commented Jul 10, 2017 via email

Uh oh!

weltling commented Jul 13, 2017

Uh oh!

flaupretre commented Jul 14, 2017

Uh oh!

weltling commented Jul 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

weltling commented Jul 7, 2017 •

edited

Loading