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

IniReader::readWithNativeFunction chokes on multi-line strings #15689

Closed
skizzerz opened this issue Mar 11, 2020 · 4 comments · Fixed by #18518
Closed

IniReader::readWithNativeFunction chokes on multi-line strings #15689

skizzerz opened this issue Mar 11, 2020 · 4 comments · Fixed by #18518
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Milestone

Comments

@skizzerz
Copy link

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
Copy link
Member

tsteur commented Mar 15, 2020

@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
Copy link
Author

@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 = @parse_ini_string($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
Copy link
Member

tsteur commented Mar 24, 2020

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 added a commit to skizzerz/component-ini that referenced this issue Mar 24, 2020
In some cases (such as when the ini file contains multi-line strings),
parse_ini_file with the INI_SCANNER_RAW flag causes the parse to fail.
This in turn leads to a lot of noise in error logs because the
subsequent call to decode throws a lot of E_NOTICEs. If the parse fails,
just use what was processed by parse_ini_file directly instead.

See matomo-org/matomo#15689 for additional context.
@skizzerz
Copy link
Author

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

@tsteur tsteur added the Bug For errors / faults / flaws / inconsistencies etc. label Mar 24, 2020
@sgiehl sgiehl added this to the 4.7.0 milestone Dec 17, 2021
@sgiehl sgiehl self-assigned this Dec 17, 2021
@justinvelluppillai justinvelluppillai added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants