search

s9y / Serendipity

A PHP blog software
https://s9y.org
BSD 3-Clause "New" or "Revised" License
203 stars 86 forks source link

"Illegal string offset" PHP warnings #642

Closed hannob closed 4 years ago

hannob commented 4 years ago

Accessing certain URLs gives me plenty of "Illegal string offset" warnings in the PHP log.

I reproduced this on a fresh installation, so I'm reasonably confident it's nothing with special extensions or special config options. This happens in all PHP versions from 7.1 to 7.4 (i.e. all currently supported ones), I haven't tested older ones.

The errors get triggered when an URL of this form is accessed:

https://[host]/index.php?url=archives/1&serendipity=true

The warnings in the log look like this:


[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'action' in [path]/serendipity_config.inc.php on line 458
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Cannot assign an empty string to a string offset in [path]/serendipity_config.inc.php on line 458
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'adminAction' in [path]/serendipity_config.inc.php on line 462
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Cannot assign an empty string to a string offset in [path]/serendipity_config.inc.php on line 462
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'action' in [path]/include/functions_routing.inc.php on line 11
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'action' in [path]/include/genpage.inc.php on line 37
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'category' in [path]/include/functions_entries.inc.php on line 213
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'hide_category' in [path]/include/functions_entries.inc.php on line 213
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'viewAuthor' in [path]/include/functions_entries.inc.php on line 213
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'page' in [path]/include/functions_entries.inc.php on line 213
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'adminModule' in [path]/include/functions_entries.inc.php on line 213
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'cview' in [path]/include/functions_entries.inc.php on line 1177
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'action' in [path]/include/functions_entries.inc.php on line 960
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'page' in [path]/include/functions_entries.inc.php on line 961
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'page' in [path]/include/functions_entries.inc.php on line 966
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'page' in [path]/include/functions_entries.inc.php on line 976
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'page' in [path]/include/functions_entries.inc.php on line 978
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'page' in [path]/include/functions_entries.inc.php on line 980
[02-Oct-2019 15:33:53 Europe/Berlin] PHP Warning:  Illegal string offset 'action' in [path]/include/genpage.inc.php on line 133
hannob commented 4 years ago

Forgot: Tested with current release version (2.3.1).

fe-hicking commented 4 years ago

Serendipity is currently not compatible with PHP E_NOTICE turned on.

Usually production versions of s9y should turn off E_NOTICE, and only alpha/testing versions should have it on. I think there are circumstances, where a PHP error_reporting is not touched, if it is specifically set on an instance.

Having said that, it would be much appreciated if Serendipity could take care of all uninitialized variables. However because it stemming from times where dynamic typing was still a valid and easy thing to do, it is much work to initialize each and every variable before use or check for its existance, which will also blow up the amount of code.

hannob commented 4 years ago

These aren't E_NOTICE warnings.

Test:


$ php -r 'error_reporting(E_ALL&~E_NOTICE);$a="";$a["x"];'
PHP Warning:  Illegal string offset 'x' in Command line code on line 1
garvinhicking commented 4 years ago

Very true. Thanks for pointing out my mistake. I just pushed a commit that should catch this type misbehaviour . The patch should also apply to older s9y releases, would you be able to confirm this fixes the warnings?

hannob commented 4 years ago

This fixes the warnings, but it introduces more PHP notices due to accessing an undefined index.

This can easily be fixed with an array_key_exists check, I'll do a pull request.

garvinhicking commented 4 years ago

@hannob I saw a major problem; in the code we actually assign $_GET['serendipity']['action'] = 'read', but the new code initializes an empty, non-referenced array for that case.

I hope the new commit properly addresses both your concern and allows proper function, could you test that on your site if it still prevents undefined index notices?

garvinhicking commented 4 years ago

(See #653 for reference)

hannob commented 4 years ago

oh yeah, sorry for my insufficient testing.

I can confirm the bug, I applied the patch and will look if new warnings appear in the logs.

hannob commented 4 years ago

I think we're done here.