Creating a just deleted region while using SQL prevents persisting any further region changes

Question

Creating a just deleted region while using SQL prevents persisting any further region changes

seyfahni opened this issue 4 years ago · 5 comments

seyfahni commented 4 years ago

Versions

WorldEdit version:

WorldEdit version 7.2.0-beta-02
https://github.com/EngineHub/WorldEdit/
------------------ Platforms ------------------
* Bukkit-Official(7.2.0-beta-02+d10d7d6)

---------------- Capabilities ----------------
GAME_HOOKS: Bukkit-Official
CONFIGURATION: Bukkit-Official
USER_COMMANDS: Bukkit-Official
PERMISSIONS: Bukkit-Official
WORLDEDIT_CUI: Bukkit-Official
WORLD_EDITING: Bukkit-Official

WorldGuard version:

WorldGuard 7.0.3
http://www.enginehub.org
----------- Platforms -----------
* Bukkit-Official (7.0.3;5407315)

Platform version:

This server is running Tuinity version git-Tuinity-"2b14f48" (MC: 1.15.2) (Implementing API version 1.15.2-R0.1-SNAPSHOT)

Bug Description

When using MySQL to persist region data the save task can be broken (intentionally). After following the reproduction steps no changes to regions can be saved and the server log is filled with exceptions what makes it hard to read.

While the MySQL persistor is in this erroneous state any further WorldGuard operation displays their usual bahavior. But stopping the server will cause an unrecoverable loss of any region data changed since the execution of the faulty command.

The main reason seems to be the system responsible for persisting data to MySQL, as it doesn't happen for YAML storage (with disabled database).

How to Reproduce

Setup a Server with WorldEdit and WorldGuard
Configure WorldGuard to use MySQL for storage
Create a region and wait until it is persisted (by default the task runs every 30 seconds)
Delete the persisted region
Create a new region with the same name as the deleted before the periodical save task is executed
Wait for the save task to be executed

Expected behaviour

Variant 1: the fix

The old region is removed and the new region is saved to the database.

Variant 2: the workaround

After the database is automatically flushed after /rg delete is executed. (Same as /rg save)

Variant 3: deprecate and/or remove

When a server is using MySQL, print a SEVERE level notification that the mysql storage is unsafe. (Maybe link to this issue?)

There should be a config option to disable this notification.

You might even decide to remove the MySQL storage support in a future version completely.

Additionally

Any failures regarding data persistence should be logged as SEVERE.

Users and hooked plugins should receive an error while the save task can not be executed successfully.

Security implications

There is an attack scenario:

Preconditions

Assuming MySQL is used as storage provider
Assuming users can create regions by themself

This is a valid and possible real scenario as it could be that some system controls the size of created regions and the fact that user regions can't overlap. This might even be done with a third party plugin.

The Attack

The malicious user can delete one of his regions and immediately recreate it. They might do it 2 times inside the time frame of 30 seconds, assuming the defaults are used. Or they might do it automatically (via scripting or a bot).

Implications

Any changes after the attack are not persisted, but users might need to pay a fee to create regions. After the next server restart their regions as well as the fee they paid will both be lost.

Only people with acces to the server log are able to spot the erroneous state, so the attack might be unspotted for some time.

As the error is logged on WARN level any system to notify system administrators if a severe failure happens is rendered useless.

Example Server Log

[16:40:53 INFO]: seyfahni issued server command: /rg create persist_test
[16:41:14 INFO]: [WorldGuard] Region data changes made in 'world' have been background saved
[16:41:16 INFO]: seyfahni issued server command: /rg delete persist_test
[16:41:17 INFO]: seyfahni issued server command: /rg create persist_test
[16:41:44 WARN]: [WorldGuard] Failed to save the region data for 'world' during a periodical save
com.sk89q.worldguard.protection.managers.storage.StorageException: Failed to save the region data to the database
	at com.sk89q.worldguard.protection.managers.storage.sql.SQLRegionDatabase.saveChanges(SQLRegionDatabase.java:270) ~[?:?]
	at com.sk89q.worldguard.protection.managers.RegionManager.saveChanges(RegionManager.java:134) ~[?:?]
	at com.sk89q.worldguard.protection.managers.RegionContainerImpl$BackgroundSaver.run(RegionContainerImpl.java:245) ~[?:?]
	at java.util.TimerThread.mainLoop(Timer.java:556) ~[?:?]
	at java.util.TimerThread.run(Timer.java:506) ~[?:?]
Caused by: java.sql.BatchUpdateException: Cannot add or update a child row: a foreign key constraint fails (`minecraft_worldguard`.`worldguard_region_cuboid`, CONSTRAINT `fk_worldguard_region_cuboid_region` FOREIGN KEY (`region_id`, `world_id`) REFERENCES `worldguard_region` (`id`, `world_id`) ON...)
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:?]
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:?]
	at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:?]
	at java.lang.reflect.Constructor.newInstance(Constructor.java:490) ~[?:?]
	at com.mysql.jdbc.Util.handleNewInstance(Util.java:425) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.Util.getInstance(Util.java:408) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.SQLError.createBatchUpdateException(SQLError.java:1154) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.PreparedStatement.executeBatchSerially(PreparedStatement.java:1832) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.PreparedStatement.executeBatchInternal(PreparedStatement.java:1316) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.StatementImpl.executeBatch(StatementImpl.java:954) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.sk89q.worldguard.protection.managers.storage.sql.RegionInserter.insertCuboids(RegionInserter.java:125) ~[?:?]
	at com.sk89q.worldguard.protection.managers.storage.sql.RegionInserter.apply(RegionInserter.java:184) ~[?:?]
	at com.sk89q.worldguard.protection.managers.storage.sql.DataUpdater.executeSave(DataUpdater.java:129) ~[?:?]
	at com.sk89q.worldguard.protection.managers.storage.sql.DataUpdater.saveChanges(DataUpdater.java:73) ~[?:?]
	at com.sk89q.worldguard.protection.managers.storage.sql.SQLRegionDatabase.saveChanges(SQLRegionDatabase.java:268) ~[?:?]
	... 4 more
Caused by: com.mysql.jdbc.exceptions.jdbc4.MySQLIntegrityConstraintViolationException: Cannot add or update a child row: a foreign key constraint fails (`minecraft_worldguard`.`worldguard_region_cuboid`, CONSTRAINT `fk_worldguard_region_cuboid_region` FOREIGN KEY (`region_id`, `world_id`) REFERENCES `worldguard_region` (`id`, `world_id`) ON...)
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:?]
	at jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:?]
	at jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:?]
	at java.lang.reflect.Constructor.newInstance(Constructor.java:490) ~[?:?]
	at com.mysql.jdbc.Util.handleNewInstance(Util.java:425) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.Util.getInstance(Util.java:408) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:936) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3933) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3869) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.MysqlIO.sendCommand(MysqlIO.java:2524) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.MysqlIO.sqlQueryDirect(MysqlIO.java:2675) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.ConnectionImpl.execSQL(ConnectionImpl.java:2465) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.PreparedStatement.executeInternal(PreparedStatement.java:1912) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.PreparedStatement.executeUpdateInternal(PreparedStatement.java:2133) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.PreparedStatement.executeBatchSerially(PreparedStatement.java:1810) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.PreparedStatement.executeBatchInternal(PreparedStatement.java:1316) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.mysql.jdbc.StatementImpl.executeBatch(StatementImpl.java:954) ~[patched_1.15.2.jar:git-Tuinity-"2b14f48"]
	at com.sk89q.worldguard.protection.managers.storage.sql.RegionInserter.insertCuboids(RegionInserter.java:125) ~[?:?]
	at com.sk89q.worldguard.protection.managers.storage.sql.RegionInserter.apply(RegionInserter.java:184) ~[?:?]
	at com.sk89q.worldguard.protection.managers.storage.sql.DataUpdater.executeSave(DataUpdater.java:129) ~[?:?]
	at com.sk89q.worldguard.protection.managers.storage.sql.DataUpdater.saveChanges(DataUpdater.java:73) ~[?:?]
	at com.sk89q.worldguard.protection.managers.storage.sql.SQLRegionDatabase.saveChanges(SQLRegionDatabase.java:268) ~[?:?]
	... 4 more

Notes

I am aware of #1504 and #1513. But as they are closed but not fixed (due to won't do), they are overlooked easily. Even if this bug is not going to be fixed by any core contributor, please leave it open. It is still an existing bug that eventually needs to be fixed or at least needs a workaround.

wizjany · Answer 1 · 2020-08-04T15:44:50.000Z

I appreciate the thorough report, but https://worldguard.enginehub.org/en/latest/regions/storage/

to be clear, "won't fix" is a perfectly valid reason to close a report. unless someone wants to open a PR fixing the issue, we don't need multiple issues about the same unsupported feature.

seyfahni · Answer 2 · 2020-08-04T15:46:15.000Z

You still might want to pin this issue, so other people that first come to GitHub to report this issue see this first.

wizjany · Answer 3 · 2020-08-04T15:47:35.000Z

There's some big assumptions there that 1) people actually read anything 2) people actually look at github 3) github is an easier place to find the information than the docs

seyfahni · Answer 4 · 2020-08-04T15:54:38.000Z

For me the first two of these assumptions true.

And the third assumption is true as bugs (unexpected giant stack traces) are usually reported inside the issue tracker and GitHub indeed does have a search function.

As I wasn't responsible for configuring WorldGuard, I hadn't read the documentation page you've sent. I've used the issue tracker link inside the documentation and searched for this issue. As I mentioned in the Notes section of my bug report I still saw a reason to open a new report even though I found other bug reports regarding this issue.

seyfahni · Answer 5 · 2020-08-04T15:59:21.000Z

Aditionally, although you warn readers to not use the MySQL storage provider, this happens only on the Storage Providers documentation page. Most people will configure WorldGuard by following the Config documentation page that does not contain any warnings regarding MySQL.

Share to