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

Potential infinite recursion when executing schema:dump #52436

Closed
FeBe95 opened this issue Aug 9, 2024 · 11 comments
Closed

Potential infinite recursion when executing schema:dump #52436

FeBe95 opened this issue Aug 9, 2024 · 11 comments

Comments

@FeBe95
Copy link

FeBe95 commented Aug 9, 2024

Laravel Version

11.20.0

PHP Version

8.2.12

Database Driver & Version

MariaDB 10.6.16

Description

I noticed that the execution of

php artisan schema:dump

can potentially run into two infinite recursive loop here:

if (Str::contains($e->getMessage(), ['column-statistics', 'column_statistics'])) {
return $this->executeDumpProcess(Process::fromShellCommandLine(
str_replace(' --column-statistics=0', '', $process->getCommandLine())
), $output, $variables);
}
if (str_contains($e->getMessage(), 'set-gtid-purged')) {
return $this->executeDumpProcess(Process::fromShellCommandLine(
str_replace(' --set-gtid-purged=OFF', '', $process->getCommandLine())
), $output, $variables);
}

There's even a comment on StackOverflow, where a user ran into exactly this issue:
https://stackoverflow.com/questions/59410746/getting-mysqldump-error-unknown-variable-column-statistics-0-when-exporting#comment136792230_59442494

I am fine with recursion in general, but there needs to be some kind of fail-safe to catch infinit loops here, e.g. a max retry count. We can't rely on consistent output of mysqldump here.

Steps To Reproduce

TLDR:

  1. Run composer create-project laravel/laravel column-statistics
  2. Setup MySQL/MariaDB database connection
  3. Set DB_USERNAME to a user that does not have the LOCK TABLES permission
  4. Run php artisan schema:dump

Output:

> php artisan schema:dump
mysqldump: unknown variable 'column-statistics=0'
mysqldump: Got error: 1044: "Access denied for user 'test'@'%' to database 'test_database'" when using LOCK TABLES
mysqldump: Got error: 1044: "Access denied for user 'test'@'%' to database 'test_database'" when using LOCK TABLES
mysqldump: Got error: 1044: "Access denied for user 'test'@'%' to database 'test_database'" when using LOCK TABLES
mysqldump: Got error: 1044: "Access denied for user 'test'@'%' to database 'test_database'" when using LOCK TABLES
mysqldump: Got error: 1044: "Access denied for user 'test'@'%' to database 'test_database'" when using LOCK TABLES
# ...

Detailed Explanation:

There are for sure multiple ways of triggering an infinite recursion here, but the simplest way, that I have found, is to have the word column-statistics somewhere in the path of your working directory. Now, if any error is reported by mysqldump, it will result in an infinite loop, since the working directory is part of the error output that is being parsed here:

if (Str::contains($e->getMessage(), ['column-statistics', 'column_statistics'])) {

In my case $e->getMessage() looked like this:

The command "mysqldump  --user="${:LARAVEL_LOAD_USER}" --password="${:LARAVEL_LOAD_PASSWORD}" --host="${:LARAVEL_LOAD_HOST}" --port="${:LARAVEL_LOAD_PORT}" --no-tablespaces --skip-add-locks --skip-comments --skip-set-charset --tz-utc "${:LARAVEL_LOAD_DATABASE}" --routines --result-file="${:LARAVEL_LOAD_PATH}" --no-data" failed.

Exit Code: 2(Misuse of shell builtins)

Working directory: C:\xampp\htdocs\column-statistics

Output:
================


Error Output:
================
mysqldump: Got error: 1044: "Access denied for user 'test'@'%' to database 'test_database'" when using LOCK TABLES
@driesvints
Copy link
Member

You're on a very old v11 version. Can you try updating all your dependencies with composer update -W and try again?

@FeBe95
Copy link
Author

FeBe95 commented Aug 9, 2024

Version 11.1.4 is apparently the version number of laravel/laravel. For laravel/framework the version number is 11.20.0. I will adjust the version number above.

P.S.: I still ran composer update -W, just to be sure, nothing to update.

Copy link

github-actions bot commented Aug 9, 2024

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@rust17
Copy link
Contributor

rust17 commented Aug 13, 2024

Hi, @FeBe95 I followed the steps, try to replicate the issue but was unsuccessful. My database version is MySQL 5.7, and the MySQL user has the following permissions:

> SHOW GRANTS FOR CURRENT_USER;

+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Grants for new_circle@%                                                                                                                                                                                                                                                                                                                                           |
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, DROP, RELOAD, SHUTDOWN, PROCESS, FILE, REFERENCES, INDEX, ALTER, SHOW DATABASES, SUPER, CREATE TEMPORARY TABLES, EXECUTE, REPLICATION SLAVE, REPLICATION CLIENT, CREATE VIEW, SHOW VIEW, CREATE ROUTINE, ALTER ROUTINE, CREATE USER, EVENT, TRIGGER, CREATE TABLESPACE ON *.* TO 'new_circle'@'%' WITH GRANT OPTION |
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

When running php artisan schema:dump, I received the following messages:

mysqldump: [Warning] Using a password on the command line interface can be insecure.
mysqldump: [ERROR] unknown variable 'column-statistics=0'
mysqldump: [Warning] Using a password on the command line interface can be insecure.

   INFO  Database schema dumped successfully.

It's there anything I missed?

@FeBe95
Copy link
Author

FeBe95 commented Aug 13, 2024

1. Replication

@rust17 I recreated a user with exactly the same grants as yours again (all except LOCK TABLES):

> SHOW GRANTS FOR 'test';

+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Grants for test@%                                                                                                                                                                                                                                                                                                                                           |
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, DROP, RELOAD, SHUTDOWN, PROCESS, FILE, REFERENCES, INDEX, ALTER, SHOW DATABASES, SUPER, CREATE TEMPORARY TABLES, EXECUTE, REPLICATION SLAVE, REPLICATION CLIENT, CREATE VIEW, SHOW VIEW, CREATE ROUTINE, ALTER ROUTINE, CREATE USER, EVENT, TRIGGER, CREATE TABLESPACE ON *.* TO 'test'@'%' WITH GRANT OPTION |
+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

and re-run php artisan schema:dump with the same results:

mysqldump: unknown variable 'column-statistics=0'
mysqldump: Got error: 1044: "Access denied for user 'test'@'%' to database 'test_database'" when using LOCK TABLES

Just to make sure, have you set DB_USERNAME=new_circle in your .env file?

2. Versions

Also, I can see that my output looks a bit different from yours. I suppose we are using different MySQL/MariaDB versions?

> mysql --version
mysql.exe Ver 15.1 Distrib 10.4.32-MariaDB, for Win64 (AMD64), source revision c4143f909528e3fab0677a28631d10389354c491
> mysqldump --version
mysqldump.exe  Ver 10.19 Distrib 10.4.32-MariaDB, for Win64 (AMD64)

3. Error Trigger

Regardless of that, LOCK TABLES is not really the issue here. I just used it to trigger an error in mysqldump during execution. This would then cause the infinite loop, if the directory itself contains column-statistics.

Basically any error, that is not caught by Laravel beforehand, will do - e.g. an incorrect password is always caught during initial setup of the PDO connection, so this would not trigger the infinite loop.

In Connector.php line 65:

  SQLSTATE[HY000] [1045] Access denied for user 'test'@'localhost' (using password: YES)

That's why I came up with removing the LOCK_TABLES grant for my user. Laravel wouldn't check this beforehand, but mysqldump would fail (at least on my machine).

@rust17
Copy link
Contributor

rust17 commented Aug 13, 2024

Just to make sure, have you set DB_USERNAME=new_circle in your .env file?

Yes. The .env file is as bellow:

DB_CONNECTION=mysql
DB_HOST=127.0.0.1
DB_PORT=3306
DB_DATABASE=laravel
DB_USERNAME=new_circle
DB_PASSWORD=password
  1. My MySQL version, I think is basically same as yours according to this:
> mysql --version
mysql  Ver 14.14 Distrib 5.7.42, for Linux (x86_64) using  EditLine wrapper

> mysqldump --version
mysqldump  Ver 10.13 Distrib 5.7.42, for Linux (x86_64)

Double-check each step in case of making any difference. On my machine, mysqldump report error as bellow, but will not result in an infinite loop:

mysqldump: [ERROR] unknown variable 'column-statistics=0'

May it's due to the Windows platform? Unfortunately, I don’t have a Windows environment right now, can’t replicate the issue.

@FeBe95
Copy link
Author

FeBe95 commented Aug 13, 2024

Hm, that's odd...

Regardless, the potential infinite recursion should also be noticeable by looking at the code directly, without any execution at all:

if (Str::contains($e->getMessage(), ['column-statistics', 'column_statistics'])) {
return $this->executeDumpProcess(Process::fromShellCommandLine(
str_replace(' --column-statistics=0', '', $process->getCommandLine())
), $output, $variables);
}

Let's convert these lines to pseudo code. For context: This code is executed, whenever mysqldump exists unexpectedly.

Pseudo Code:

IF error message contains either "column-statistics" or "column_statistics" THEN
    remove " --column-statistics=0" from the command line and recursively execute the dump command again
END IF

This code could potentially result in an infinite recursion, whenever the removal of the flag does not fix the actual error.

In my forced example the error message contains

'column-statistics'

but not exactly

' --column-statistics=0'

Here the actual error does not complain about an unknown flag, but is completely unrelated, while the error output happens to contain the searched string (as a working directory).


Same goes for the second check further below:

if (str_contains($e->getMessage(), 'set-gtid-purged')) {
return $this->executeDumpProcess(Process::fromShellCommandLine(
str_replace(' --set-gtid-purged=OFF', '', $process->getCommandLine())
), $output, $variables);
}

mysqldump might report something completely unrelated about set-gtid-purged. Re-running the command without the --set-gtid-purged=OFF flag might not fix the problem - again resulting in an infinite loop.

In my case I forced this by naming my directory column-statistics, because the error message of mysqldump will also contain the working directory in its output (see "Detailed Explanation" in the issue's description above).

@rust17
Copy link
Contributor

rust17 commented Aug 13, 2024

I think I get what you’re saying. Regardless of how the schema:dump command is altered, it will always result in an error. Given this, if the command is executed and the project directory contains keywords like column-statistics, column_statistics, or set-gtid-purged, it will keep executing schema:dump again, causing an infinite loop. Therefore, the recommendation is to exclude the project directory from the error message first, and then proceed with keyword matching, correct?

@FeBe95
Copy link
Author

FeBe95 commented Aug 13, 2024

Yes! That is exactly what I meant. 😊

Regarding your solution: The working directory is only one possible cause, that I came up with for the sake of this issue. There are other possible causes for an infinite loop besides the working directory though.

For example, mentioned here on StackOverflow, one can create a config file for mysqldump located in the user folder. If this file contains column-statistics=0, the execution would then again fail with the same error, regardless of Laravel using or omitting the flag.

I know, these issues might be somewhat unlikely to happen, but I think having even the sightliest possibility to run in an infinite loop somewhere in the code is "bad design".

So excluding the working directory from the error message is a step in the right direction, but I would advocate preventing any infinite loop here, by e.g. introducing a MAX_RETRIES constant or something similar.

@rust17
Copy link
Contributor

rust17 commented Aug 14, 2024

Thanks. To make this code more robust, it's better to add some logic to ensure that the recursion has a clear stopping point. I’ll try to submit a pull request for this.

taylorotwell added a commit that referenced this issue Aug 15, 2024
* feat: Add clear stopping point for recursion

* style: fix styleci

* Update MySqlSchemaState.php

* Update DatabaseMySqlSchemaStateTest.php

* Update MySqlSchemaState.php

---------

Co-authored-by: Taylor Otwell <[email protected]>
@driesvints
Copy link
Member

Thanks @rust17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants