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

[Dotenv] Fixed infinite loop with missing quote followed by quoted value #34643

Merged
merged 1 commit into from Jan 8, 2020

Conversation

@naitsirch
Copy link
Contributor

naitsirch commented Nov 27, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #34642
License MIT
Doc PR

If there's a quote missing to end a value and in the next line there's again a quoted value Dotenv will run into an infinite loop. An .env file with the following content will result in this error:

FOO="foo
BAR="bar"

See #34642 for more details.

@bykclk
bykclk approved these changes Dec 4, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 10, 2019

I'm not sure about the patch: if I revert the change but keep the test, the test fails with an undefined offset error.
This error is fixed by this patch:

--- a/src/Symfony/Component/Dotenv/Dotenv.php
+++ b/src/Symfony/Component/Dotenv/Dotenv.php
@@ -203,7 +203,10 @@ final class Dotenv
                 $this->cursor += 1 + $len;
             } elseif ('"' === $this->data[$this->cursor]) {
                 $value = '';
-                ++$this->cursor;
+
+                if (++$this->cursor === $this->end) {
+                    throw $this->createFormatException('Missing quote to end the value');
+                }
 
                 while ('"' !== $this->data[$this->cursor] || ('\\' === $this->data[$this->cursor - 1] && '\\' !== $this->data[$this->cursor - 2])) {
                     $value .= $this->data[$this->cursor];

Which means we need another test case to trigger the infinite loop and fix it I think.

@naitsirch

This comment has been minimized.

Copy link
Contributor Author

naitsirch commented Dec 10, 2019

I'm not sure about the patch: if I revert the change but keep the test, the test fails with an undefined offset error.

@nicolas-grekas Yes, this is because of the ErrorHandler of phpunit. The access of the uninitialized string offset triggers a notice which invokes the error handler and throws an exception. To trigger the infinite loop in the unit test you have to disable phpunit's convertNoticesToExceptions setting.

Or if you want to see the infinite loop you can try the code block that I have attached to #34642.

This error is fixed by this patch:

Your patch fixes the error, but with a wrong offset in the error hint.
Try this script with your fix:

require __DIR__.'/vendor/autoload.php';

$testString = 'FOO="x"
DOO="y
VOO="z"';

$dotenv = new \Symfony\Component\Dotenv\Dotenv();

$dotenv->parse($testString);

the output will look like:

$ php test.php 
PHP Fatal error:  Uncaught Symfony\Component\Dotenv\Exception\FormatException: Missing quote to end the value in ".env" at line 2.
...O="x"\nDOO="y\nVOO="z"...
                        ^ line 2 offset 22 in /var/www/test-dotenv/vendor/symfony/symfony/src/Symfony/Component/Dotenv/Dotenv.php:426

This makes it harder to find the broken line, because the hint in the output points to the last character of the whole env file.
This is why I have introduced the $prevLf variable.

My patch will output

$ php test.php 
PHP Fatal error:  Uncaught Symfony\Component\Dotenv\Exception\FormatException: Missing quote to end the value in ".env" at line 2.
...FOO="x"\nDOO="y\nVOO="z"...
                 ^ line 2 offset 14 in /var/www/test-dotenv/vendor/symfony/symfony/src/Symfony/Component/Dotenv/Dotenv.php:426

I think this makes it easier to spot the issue.

@naitsirch

This comment has been minimized.

Copy link
Contributor Author

naitsirch commented Dec 10, 2019

Even better would be if the output sayd line 2 offset 6 but for this to work I'd have to use another way to throw the exception.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 15, 2019

Your patch fixes the error, but with a wrong offset in the error hint.

The error looks correct to me. Multi-line quoted strings are possible, we cannot assume anything about their content.

@naitsirch

This comment has been minimized.

Copy link
Contributor Author

naitsirch commented Dec 20, 2019

Okay, how should be proceed with this PR? Should I revert my fix and only provide the unit test, or should I change my fix to the version suggested by you?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 20, 2019

I'd be in favor of adopting my patch if you don't mind too much, and adapt tests accordingly.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 4, 2020

ping @naitsirch

@naitsirch

This comment has been minimized.

Copy link
Contributor Author

naitsirch commented Jan 4, 2020

Okay, I'll try to update my PR and will contact you in the next few days.

@naitsirch naitsirch force-pushed the naitsirch:fix-34642 branch from a51c99b to 1ab91dc Jan 7, 2020
If there's a quote missing to end a value and in the next line there's again a quoted value Dotenv will run into an infinite loop. An .env file with the following content will result in this error:
```
FOO="foo
BAR="bar"
```
See #34642 for more details.
@naitsirch naitsirch force-pushed the naitsirch:fix-34642 branch from 1ab91dc to eb69e13 Jan 7, 2020
@naitsirch

This comment has been minimized.

Copy link
Contributor Author

naitsirch commented Jan 7, 2020

@nicolas-grekas I have updated the patch following your suggestion and adjusted the unit test.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 8, 2020

Thank you @naitsirch.

nicolas-grekas added a commit that referenced this pull request Jan 8, 2020
…y quoted value (naitsirch)

This PR was merged into the 3.4 branch.

Discussion
----------

[Dotenv] Fixed infinite loop with missing quote followed by quoted value

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34642
| License       | MIT
| Doc PR        |

If there's a quote missing to end a value and in the next line there's again a quoted value Dotenv will run into an infinite loop. An .env file with the following content will result in this error:
```
FOO="foo
BAR="bar"
```
See #34642 for more details.

Commits
-------

eb69e13 [Dotenv] Fixed infinite loop with missing quote followed by quoted value
@nicolas-grekas nicolas-grekas merged commit eb69e13 into symfony:3.4 Jan 8, 2020
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.