Mineclonia
/
Mineclonia
Archived
12
17
Fork 13
Code Issues 105 Pull Requests 19 Projects Releases Wiki Activity

ENTITIES/mcl_item_entity: Fix non-serializable item entity unload crash #132

Merged
cora merged 3 commits from fix-overlong-meta-item-crash-2 into master 2021-08-01 03:23:44 +02:00
Conversation 2 Commits 3 Files Changed 1 +61 -1
erlehmann commented 2021-07-30 17:25:10 +02:00
Owner
Copy Link
Problem

TRACKING ISSUE: #129

Some items, like shulkers or books, can have so much metadata that the
corresponding item entity can not be serialized by the Minetest engine.

Without this patch, dropping such an item and then moving away crashes
Minetest, as it can not serialize the entity with serializeString16()
when unloading a map block.

Solution

The patch resets the overlong metadata of non-serializable item entities.
This avoids a crash and makes it possible to retrieve a “sanitized” item
without metadata when the mapblock containing the item entity is reloaded.

Details

The modified get_staticdata() function was given to me by sfan5 on IRC.

I also got the permission to use the code “under any license you want”:
https://irc.minetest.net/minetest/2021-07-28#i_5853853

The original threshold for serialization size given by sfan5 was wrong.

@anon5 calculated the correct size here: #131 (comment)

Testing Steps
Preparation
  1. Create a world with mapgen v7 with the seed crashbook.
  2. Enter the world in Mineclonia commit ce6d6c26cc.
  3. Enter the following lines in the chat/terminal:
    • /grantme debug
    • /grantme fly
    • /grantme give
    • /grantme teleport
  4. Verify that the game shows the player “singleplayer” to have the privileges debug, fly, give, and teleport.
  5. Press the fly key.
  6. Verify that the game shows a message that fly mode has been activated.
  7. Enter the following lines in the chat/terminal:
    • /teleport 0,28,0
    • /getwrittenbook 65323
    • /getwrittenbook 65323
    • /getwrittenbook 65324
    • /giveme mcl_chests:violet_shulker_box
  8. Verify that you are floating above the ground.
  9. Verify that your inventory contains a stack of two books, a stack of one book, and a shulker chest.
Verify Bug
  1. Enter the prepared world in Mineclonia commit 45cdad7283.
  2. Drop the first book in your inventory while pressing sneak.
  3. Verify that your inventory contains a stack of one book, a stack of one book, and a shulker chest.
  4. Enter the following line in the chat/terminal:
  • /teleport 0,300,0
  1. Wait 20 seconds.
  2. Verify that Minetest did not crash.
  3. Enter the following line in the chat/terminal:
  • /teleport 0,28,0
  1. Drop the second book in your inventory.
  2. Enter the following line in the chat/terminal:
  • /teleport 0,300,0
  1. Wait 20 seconds.
  2. Verify that Minetest crashed with an error message containing “String too long for serializeString16”.
  3. Enter the prepared world in Mineclonia commit 45cdad7283.
  4. Enter the following line in the chat/terminal:
  • /teleport 0,28,0
  1. Place the shulker box on the ground.
  2. Place the remaining book in the shulker box.
  3. Dig the shulker box, but do not pick it up. If you pick it up, drop it.
  4. Enter the following line in the chat/terminal:
  • /teleport 0,300,0
  1. Wait 20 seconds.
  2. Verify that Minetest crashed with an error message containing “String too long for serializeString16”.
Verify Patch
  1. Enter the prepared world in Mineclonia commit ce6d6c26cc.
  2. Drop the first book in your inventory while pressing sneak.
  3. Verify that your inventory contains a stack of one book, a stack of one book, and a shulker chest.
  4. Enter the following line in the chat/terminal:
  • /teleport 0,300,0
  1. Wait 20 seconds.
  2. Verify that Minetest did not crash.
  3. Enter the following line in the chat/terminal:
  • /teleport 0,28,0
  1. Right click while holding the second book in your inventory pointing at nothing.
  2. Verify that the book contains a lot of repeated “x” characters. You might experience lag.
  3. Close the book form.
  4. Drop the second book in your inventory.
  5. Enter the following line in the chat/terminal:
  • /teleport 0,300,0
  1. Wait 20 seconds.
  2. Verify that Minetest did not crash.
  3. Verify that the Minetest contains has a single warning that reports “Overlong item entity metadata removed” for an item “mcl_books:written book” with a serialized length greater then 65535.
  4. Enter the following line in the chat/terminal:
  • /teleport 0,28,0
  1. Pick up the second book you dropped.
  2. Right click while holding the second book in your inventory pointing at nothing.
  3. Verify that the book contains no text.
  4. Place the shulker box on the ground.
  5. Place all books you have in the shulker box.
  6. Dig the shulker box and pick it up.
  7. Place the shulker box.
  8. Verify that the shulker box still contains all the books you put in.
  9. Dig the shulker box and do not pick it up. If you pick it up, drop it.
  10. Enter the following line in the chat/terminal:
  • /teleport 0,300,0
  1. Wait 20 seconds.
  2. Verify that Minetest did not crash.
  3. Verify that the Minetest log contains a single warning that reports “Overlong item entity metadata removed” for an item “mcl_chests:violet_shulker_box” with a serialized length greater then 65535.
  4. Enter the following line in the chat/terminal:
  • /teleport 0,28,0
  1. Pick up the shulker box.
  2. Place the shulker box on the ground.
  3. Verify that the shulker box is empty.
To do
  • Fill out issue template
  • Write testing instructions
  • Verify testing instructions
##### Problem TRACKING ISSUE: https://git.minetest.land/Mineclonia/Mineclonia/issues/129 Some items, like shulkers or books, can have so much metadata that the corresponding item entity can not be serialized by the Minetest engine. Without this patch, dropping such an item and then moving away crashes Minetest, as it can not serialize the entity with serializeString16() when unloading a map block. ##### Solution The patch resets the overlong metadata of non-serializable item entities. This avoids a crash and makes it possible to retrieve a “sanitized” item without metadata when the mapblock containing the item entity is reloaded. ##### Details The modified get_staticdata() function was given to me by sfan5 on IRC. I also got the permission to use the code “under any license you want”: https://irc.minetest.net/minetest/2021-07-28#i_5853853 The original threshold for serialization size given by sfan5 was wrong. @anon5 calculated the correct size here: https://git.minetest.land/Mineclonia/Mineclonia/pulls/131#issuecomment-26196 ##### Testing Steps ###### Preparation 1. Create a world with mapgen v7 with the seed crashbook. 2. Enter the world in Mineclonia commit ce6d6c26cc00639876bdccf6421f5a8f1e00fa94. 3. Enter the following lines in the chat/terminal: - /grantme debug - /grantme fly - /grantme give - /grantme teleport 4. Verify that the game shows the player “singleplayer” to have the privileges debug, fly, give, and teleport. 5. Press the fly key. 6. Verify that the game shows a message that fly mode has been activated. 7. Enter the following lines in the chat/terminal: - /teleport 0,28,0 - /getwrittenbook 65323 - /getwrittenbook 65323 - /getwrittenbook 65324 - /giveme mcl_chests:violet_shulker_box 8. Verify that you are floating above the ground. 9. Verify that your inventory contains a stack of two books, a stack of one book, and a shulker chest. ###### Verify Bug 10. Enter the prepared world in Mineclonia commit 45cdad72832816357e22318ef45ed408ad5154d5. 11. Drop the first book in your inventory while pressing sneak. 12. Verify that your inventory contains a stack of one book, a stack of one book, and a shulker chest. 13. Enter the following line in the chat/terminal: - /teleport 0,300,0 14. Wait 20 seconds. 15. Verify that Minetest did not crash. 16. Enter the following line in the chat/terminal: - /teleport 0,28,0 17. Drop the second book in your inventory. 18. Enter the following line in the chat/terminal: - /teleport 0,300,0 19. Wait 20 seconds. 20. Verify that Minetest crashed with an error message containing “String too long for serializeString16”. 21. Enter the prepared world in Mineclonia commit 45cdad72832816357e22318ef45ed408ad5154d5. 22. Enter the following line in the chat/terminal: - /teleport 0,28,0 23. Place the shulker box on the ground. 24. Place the remaining book in the shulker box. 25. Dig the shulker box, but do not pick it up. If you pick it up, drop it. 25. Enter the following line in the chat/terminal: - /teleport 0,300,0 26. Wait 20 seconds. 27. Verify that Minetest crashed with an error message containing “String too long for serializeString16”. ###### Verify Patch 10. Enter the prepared world in Mineclonia commit ce6d6c26cc00639876bdccf6421f5a8f1e00fa94. 11. Drop the first book in your inventory while pressing sneak. 12. Verify that your inventory contains a stack of one book, a stack of one book, and a shulker chest. 13. Enter the following line in the chat/terminal: - /teleport 0,300,0 14. Wait 20 seconds. 15. Verify that Minetest did not crash. 16. Enter the following line in the chat/terminal: - /teleport 0,28,0 17. Right click while holding the second book in your inventory pointing at nothing. 17. Verify that the book contains a lot of repeated “x” characters. You might experience lag. 17. Close the book form. 17. Drop the second book in your inventory. 18. Enter the following line in the chat/terminal: - /teleport 0,300,0 19. Wait 20 seconds. 20. Verify that Minetest did not crash. 21. Verify that the Minetest contains has a single warning that reports “Overlong item entity metadata removed” for an item “mcl_books:written book” with a serialized length greater then 65535. 22. Enter the following line in the chat/terminal: - /teleport 0,28,0 22. Pick up the second book you dropped. 22. Right click while holding the second book in your inventory pointing at nothing. 22. Verify that the book contains no text. 23. Place the shulker box on the ground. 24. Place all books you have in the shulker box. 25. Dig the shulker box and pick it up. 26. Place the shulker box. 27. Verify that the shulker box still contains all the books you put in. 28. Dig the shulker box and do not pick it up. If you pick it up, drop it. 25. Enter the following line in the chat/terminal: - /teleport 0,300,0 26. Wait 20 seconds. 27. Verify that Minetest did not crash. 27. Verify that the Minetest log contains a single warning that reports “Overlong item entity metadata removed” for an item “mcl_chests:violet_shulker_box” with a serialized length greater then 65535. 28. Enter the following line in the chat/terminal: - /teleport 0,28,0 29. Pick up the shulker box. 30. Place the shulker box on the ground. 31. Verify that the shulker box is empty. ##### To do - [x] Fill out issue template - [x] Write testing instructions - [x] Verify testing instructions
erlehmann added 3 commits 2021-07-30 17:25:12 +02:00
62d5b547a0
Fix non-serializable item entity unload crash 
Some items, like shulkers or books, can have so much metadata that the
corresponding item entity can not be serialized by the Minetest engine.

Without this patch, dropping such an item and then moving away crashes
Minetest, as it can not serialize the entity with serializeString16()
when unloading a map block.

The patch resets the overlong metadata of non-serializable item entities.
This avoids a crash and makes it possible to retrieve a “sanitized” item
without metadata when the mapblock containing the item entity is reloaded.

Originally sfan5 guessed the maximum possible item entity serialization size
that would not lead to a crash as 65530 bytes, but anon5 calculated it to be
actually 65487 bytes. This has been experimentally verified by erlehmann.
ce6d6c26cc
Add debug command to acquire a written book 
The “getwrittenbook” command gives a player that has the “debug” privilege a book
with a configurable amount of characters. This was added as a debug aid, to help
reproducing situations in which items with lots of metadata trigger issues like
heavy lag or server crashes.
erlehmann changed title from WIP: ENTITIES/mcl_item_entity: Fix non-serializable item entity unload crash to ENTITIES/mcl_item_entity: Fix non-serializable item entity unload crash 2021-07-30 19:31:04 +02:00
erlehmann commented 2021-07-30 19:34:46 +02:00
Author
Owner
Copy Link

@anon5 does this PR look correct?

@anon5 does this PR look correct?
erlehmann commented 2021-07-30 19:55:17 +02:00
Author
Owner
Copy Link

@Li0n mentioned that it should be tested if a book or shulker gets its inventory deleted immediately or only on unload. I modified the testing instructions accordingly.

@Li0n mentioned that it should be tested if a book or shulker gets its inventory deleted immediately or only on unload. I modified the testing instructions accordingly.
cora approved these changes 2021-08-01 03:23:01 +02:00
cora left a comment
Member

Preparation

  • Create a world with mapgen v7 with the seed crashbook.
  • Enter the world in Mineclonia commit ce6d6c26cc.
  • Enter the following lines in the chat/terminal:
    [...]
  • Verify that the game shows the player “singleplayer” to have the privileges debug, fly, give, and teleport.
  • Press the fly key.
  • Verify that the game shows a message that fly mode has been activated.
  • Enter the following lines in the chat/terminal:
    [...]
  • Verify that you are floating above the ground.
  • Verify that your inventory contains a stack of two books, a stack of one book, and a shulker chest.

Verify Bug

  • Enter the prepared world in Mineclonia commit 45cdad7283.
  • Drop the first book in your inventory while pressing sneak.
  • Verify that your inventory contains a stack of one book, a stack of one book, and a shulker chest.
  • Enter the following line in the chat/terminal: /teleport 0,300,0
  • Wait 20 seconds.
  • Verify that Minetest did not crash.
  • Enter the following line in the chat/terminal: /teleport 0,28,0
  • Drop the second book in your inventory.
  • Enter the following line in the chat/terminal: /teleport 0,300,0
  • Wait 20 seconds.
  • Verify that Minetest crashed with an error message containing “String too long for serializeString16”.
  • Enter the prepared world in Mineclonia commit 45cdad7283.
  • Enter the following line in the chat/terminal: /teleport 0,28,0
  • Place the shulker box on the ground.
  • Place the remaining book in the shulker box.
  • Dig the shulker box, but do not pick it up. If you pick it up, drop it.
  • Enter the following line in the chat/terminal:
  • Wait 20 seconds.
  • Verify that Minetest crashed with an error message containing “String too long for serializeString16”.

Verify Patch

  • Enter the prepared world in Mineclonia commit t.
  • Drop the first book in your inventory while pressing sneak.
  • Verify that your inventory contains a stack of one book, a stack of one book, and a shulker chest.
  • Enter the following line in the chat/terminal: /teleport 0,300,0
  • Wait 20 seconds.
  • Verify that Minetest did not crash.
  • Enter the following line in the chat/terminal: /teleport 0,28,0
  • Right click while holding the second book in your inventory pointing at nothing.
  • Verify that the book contains a lot of repeated “x” characters. You might experience lag.
  • Close the book form.
  • Drop the second book in your inventory.
  • Enter the following line in the chat/terminal: /teleport 0,300,0
  • Wait 20 seconds.
  • Verify that Minetest did not crash.
  • Verify that the Minetest contains has a single warning that reports “Overlong item entity metadata removed” for an item “mcl_books:written book” with a serialized length greater then 65535.
  • Enter the following line in the chat/terminal: /teleport 0,28,0
  • Pick up the second book you dropped.
  • Right click while holding the second book in your inventory pointing at nothing.
  • Verify that the book contains no text.
  • Place the shulker box on the ground.
  • Place all books you have in the shulker box.
  • Dig the shulker box and pick it up.
  • Place the shulker box.
  • Verify that the shulker box still contains all the books you put in.
  • Dig the shulker box and do not pick it up. If you pick it up, drop it.
  • Enter the following line in the chat/terminal: /teleport 0,300,0
  • Wait 20 seconds.
  • Verify that Minetest did not crash.
  • Verify that the Minetest log contains a single warning that reports “Overlong item entity metadata removed” for an item “mcl_chests:violet_shulker_box” with a serialized length greater then 65535.
  • Enter the following line in the chat/terminal: /teleport 0,28,0
  • Pick up the shulker box.
  • Place the shulker box on the ground.
  • Verify that the shulker box is empty
Preparation - [x] Create a world with mapgen v7 with the seed crashbook. - [x] Enter the world in Mineclonia commit ce6d6c26cc. - [x] Enter the following lines in the chat/terminal: [...] - [x] Verify that the game shows the player “singleplayer” to have the privileges debug, fly, give, and teleport. - [x] Press the fly key. - [x] Verify that the game shows a message that fly mode has been activated. - [x] Enter the following lines in the chat/terminal: [...] - [x] Verify that you are floating above the ground. - [x] Verify that your inventory contains a stack of two books, a stack of one book, and a shulker chest. Verify Bug - [x] Enter the prepared world in Mineclonia commit 45cdad7283. - [x] Drop the first book in your inventory while pressing sneak. - [x] Verify that your inventory contains a stack of one book, a stack of one book, and a shulker chest. - [x] Enter the following line in the chat/terminal: /teleport 0,300,0 - [x] Wait 20 seconds. - [x] Verify that Minetest did not crash. - [x] Enter the following line in the chat/terminal: /teleport 0,28,0 - [x] Drop the second book in your inventory. - [x] Enter the following line in the chat/terminal: /teleport 0,300,0 - [x] Wait 20 seconds. - [x] Verify that Minetest crashed with an error message containing “String too long for serializeString16”. - [x] Enter the prepared world in Mineclonia commit 45cdad7283. - [x] Enter the following line in the chat/terminal: /teleport 0,28,0 - [x] Place the shulker box on the ground. - [x] Place the remaining book in the shulker box. - [x] Dig the shulker box, but do not pick it up. If you pick it up, drop it. - [x] Enter the following line in the chat/terminal: - [x] Wait 20 seconds. - [x] Verify that Minetest crashed with an error message containing “String too long for serializeString16”. Verify Patch - [x] Enter the prepared world in Mineclonia commit t. - [x] Drop the first book in your inventory while pressing sneak. - [x] Verify that your inventory contains a stack of one book, a stack of one book, and a shulker chest. - [x] Enter the following line in the chat/terminal: /teleport 0,300,0 - [x] Wait 20 seconds. - [x] Verify that Minetest did not crash. - [x] Enter the following line in the chat/terminal: /teleport 0,28,0 - [x] Right click while holding the second book in your inventory pointing at nothing. - [x] Verify that the book contains a lot of repeated “x” characters. You might experience lag. - [x] Close the book form. - [x] Drop the second book in your inventory. - [x] Enter the following line in the chat/terminal: /teleport 0,300,0 - [x] Wait 20 seconds. - [x] Verify that Minetest did not crash. - [x] Verify that the Minetest contains has a single warning that reports “Overlong item entity metadata removed” for an item “mcl_books:written book” with a serialized length greater then 65535. - [x] Enter the following line in the chat/terminal: /teleport 0,28,0 - [x] Pick up the second book you dropped. - [x] Right click while holding the second book in your inventory pointing at nothing. - [x] Verify that the book contains no text. - [x] Place the shulker box on the ground. - [x] Place all books you have in the shulker box. - [x] Dig the shulker box and pick it up. - [x] Place the shulker box. - [x] Verify that the shulker box still contains all the books you put in. - [x] Dig the shulker box and do not pick it up. If you pick it up, drop it. - [x] Enter the following line in the chat/terminal: /teleport 0,300,0 - [x] Wait 20 seconds. - [x] Verify that Minetest did not crash. - [x] Verify that the Minetest log contains a single warning that reports “Overlong item entity metadata removed” for an item “mcl_chests:violet_shulker_box” with a serialized length greater then 65535. - [x] Enter the following line in the chat/terminal: /teleport 0,28,0 - [x] Pick up the shulker box. - [x] Place the shulker box on the ground. - [x] Verify that the shulker box is empty
cora merged commit 3cd4ad5591 into master 2021-08-01 03:23:44 +02:00
cora deleted branch fix-overlong-meta-item-crash-2 2021-08-01 03:24:03 +02:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
cora
Labels
Clear labels   
blocker

blocks release until resolved

  
bug

   
code quality

   
confirmed

   
critical

crashes, catastrophic bugs

  
discussion

   
high priority

   
incompatibility

incompatible with mcl2

  
incomplete feature

   
invalid

   
low priority

   
missing feauture

   
needs testing

   
packet spam

   
performance

   
project

meta-issues

  
regression

   
translations

   
unconfirmed
  
in review

   
ready for review
No Label
blocker
bug
code quality
confirmed
critical
discussion
high priority
incompatibility
incomplete feature
invalid
low priority
missing feauture
needs testing
packet spam
performance
project
regression
translations
unconfirmed
in review
ready for review
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Mineclonia/Mineclonia#132
No description provided.
Delete Branch "fix-overlong-meta-item-crash-2"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?