@skizzerz opened this Issue on March 11th 2020

parse_ini_string($ini, true, INI_SCANNER_RAW) returns false if the ini string being parsed contains a multi-line quoted string, citing a syntax error in the ini. Without INI_SCANNER_RAW the parse succeeds and it properly reads the multi-line string.

While this is quite possibly a bug in PHP itself, it's causing a lot of noise in the error logs because matomo isn't checking the return value of this function call and just assumes it succeeds.

To easily reproduce, just have some configuration which requires multiple lines. In my case, it was the LoginSaml configuration to specify server certificates (idp_x509cert, advanced_sp_x509cert, and advanced_sp_privatekey). However, you can probably reproduce this by manually editing the file and just inserting any arbitrary multi-line configuration

@tsteur commented on March 15th 2020 Member

@skizzerz what messages appears in your logs? And does Matomo otherwise still function for you?

If you have an example config that be great too. Or I assume it be like

[foo]
bar = "baz
baz"

?

@skizzerz commented on March 23rd 2020

@tsteur Matomo still functions normally as far as I can tell because it falls back to what was read from the initial parse_ini_string() call, so this is really only causing lots of log noise.

The logs all look like the following (sometimes the type is bool instead of null)

PHP Notice:  Trying to access array offset on value of type null in /var/www/matomo/vendor/piwik/ini/src/IniReader.php on line 352

Line number may be a bit off because I manually patched the file to suppress the warning in the meantime. The error is referring to this line of code: https://github.com/matomo-org/component-ini/blob/master/src/IniReader.php#L353

My patch was in the readNativeFunction method:

        $rawValues = <a class='mention' href='https://github.com/parse_ini_string'>@parse_ini_string</a>($ini, true, INI_SCANNER_RAW);
        if ($rawValues === false) {
                return $this->decode($array, $array);
        }
        $array = $this->decode($array, $rawValues);

I can submit that as a PR if desired, but I held off on that because of the doc comments in decode() about the default parse_ini_file method not working correctly in some instances. As this would effectively use that method without checking values, it may introduce regressions in other areas (namely, whatever cased you to start using the raw scanner and decode method in the first place).

That config you posted should reproduce the issue.

@tsteur commented on March 24th 2020 Member

I can confirm the issue.

I was thinking disabling the method parse_ini_string might help (as a worst case workaround) but noticed it doesn't work then either as the non-native implementation doesn't suppose this feature by the looks.

I think your patch should work and be safe.

Looking at http://3v4l.org/m24cT it might not even be needed anymore in general since it only affected older versions of PHP by the looks. Would you mind creating a PR for our 4.x-dev branch?

@skizzerz commented on March 24th 2020

The file was part of a composer component, PR opened against the dev branch of that repo instead.

Powered by GitHub Issue Mirror