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

ITEMS/REDSTONE/mcl_comparators: Fix redstone comparator flooding crash #284

Merged
cora merged 2 commits from fix-comparator-cauldron-water-crash into master 2022-02-23 23:30:47 +01:00
Conversation 11 Commits 2 Files Changed 3 +16 -3
erlehmann commented 2022-02-21 17:27:04 +01:00
Owner
Copy Link
Problem

TRACKING ISSUE: #283

Redstone comparators have two modes, comparison mode & subtraction mode.
Before this patch, the functions to turn comparators on or off attempted
to swap nodes with comparators in the same mode, but failed to determine
the correct replacement node, if the existing node was not a comparator.

When a comparator in an on state (e.g. powered by a filled cauldron) was
flooded, the flooding dropped the comparator and replaced the comparator
node that was to be swapped out with air, which lead to a server crash.

Solution

This patch changes the functions that turn comparators on or off so they
only swap existing nodes with comparators in the same mode if the name
of the replacement node can be determined – i.e. if it is not nil.

Details

The patch uses minetest.after(). This means there exists a race condition:
If the server shuts down during the 0.1 seconds where a comparator is turned
on or off, the on/off state of the comparator might be bugged in the future.

Testing Steps
Verify Bug
  1. Enter a world in Minetest 5.4.1 or Minetest 5.5 with Mineclonia commit d9764fdac6.
  2. Give yourself debug privilege with the command /grantme debug.
  3. Spawn the comparator test structure using /spawnstruct test_structure_comparator while looking west.
  4. Fill the cauldron using a water bucket (mcl_buckets:bucket_water).
  5. Place water using a water bucket on the redstone lamp, so that the comparator is flooded.
  6. Verify that the Minetest server crashes with a message similar to the following:
2022-02-21 16:22:22: ERROR[Main]: ServerError: AsyncErr: Lua: Runtime error from mod 'mcl_comparators' in callback LuaABM::trigger(): Node name is not set or is not a string!
2022-02-21 16:22:22: ERROR[Main]: stack traceback:
2022-02-21 16:22:22: ERROR[Main]: 	[C]: in function 'swap_node'
2022-02-21 16:22:22: ERROR[Main]: 	...es/Mineclonia/mods/ITEMS/REDSTONE/mcl_observers/init.lua:344: in function 'swap_node'
2022-02-21 16:22:22: ERROR[Main]: 	.../Mineclonia/mods/ITEMS/REDSTONE/mcl_comparators/init.lua:47: in function 'comparator_activate'
2022-02-21 16:22:22: ERROR[Main]: 	.../Mineclonia/mods/ITEMS/REDSTONE/mcl_comparators/init.lua:127: in function <.../Mineclonia/mods/ITEMS/REDSTONE/mcl_comparators/init.lua:121>
Verify Patch

Prerequisites

  1. Enter a world in Minetest 5.4.1 or Minetest 5.5 with Mineclonia commit be915478af.
  2. Give yourself debug privilege with the command /grantme debug.
  3. Acquire some water buckets (mcl_buckets:bucket_water) and empty buckets (mcl_buckets:bucket_empty).

Verify that the crash is fixed

  1. Spawn the comparator test structure using /spawnstruct test_structure_comparator while looking west.
  2. Fill the cauldron using a water bucket.
  3. Place water on the redstone lamp using a water bucket, so that the comparator is flooded.
  4. Verify that the Minetest server does not crash and the comparator drops as an item (mcl_comparators:comparator_off_comp).

Verify comparator still works in comparison mode

  1. Spawn the comparator test structure using /spawnstruct test_structure_comparator.
  2. Fill the cauldron using a water bucket.
  3. Verify that the comparator gets turned into a turned-on comparator in comparison mode (mcl_comparators:comparator_on_comp) and that a short time later the redstone lamp turns on (mesecons_lightstone:lightstone_on).
  4. Empty the cauldron using an empty bucket.
  5. Verify that the comparator gets turned into a turned-off comparator in comparison mode (mcl_comparators:comparator_off_comp) and that a short time later the redstone lamp turns off (mesecons_lightstone:lightstone_off)

Verify comparator still works in subtraction mode

  1. Spawn the comparator test structure using /spawnstruct test_structure_comparator.
  2. Right click the comparator.
  3. Verify that the comparator gets turned into a turned-off comparator in subtraction mode (mcl_comparators:comparator_off_sub).
  4. Fill the cauldron using a water bucket.
  5. Verify that the comparator gets turned into a turned-on comparator in subtraction mode (mcl_comparators:comparator_on_sub) and that a short time later the redstone lamp turns on.
  6. Press the button. Verify that the comparator and the lamp turn off and on again (with a small delay for the lamp).
  7. Flip the switch. Verify that the comparator and the lamp turn both off (with a small delay for the lamp).
  8. Flip the switch again. Verify that the comparator and the lamp turn on (with a small delay for the lamp).
  9. Empty the cauldron using an empty bucket.
  10. Verify that the comparator gets turned into a turned-off comparator in subtraction mode and that a short time later the redstone lamp turns off.
##### Problem TRACKING ISSUE: https://git.minetest.land/Mineclonia/Mineclonia/issues/283 Redstone comparators have two modes, comparison mode & subtraction mode. Before this patch, the functions to turn comparators on or off attempted to swap nodes with comparators in the same mode, but failed to determine the correct replacement node, if the existing node was not a comparator. When a comparator in an on state (e.g. powered by a filled cauldron) was flooded, the flooding dropped the comparator and replaced the comparator node that was to be swapped out with air, which lead to a server crash. ##### Solution This patch changes the functions that turn comparators on or off so they only swap existing nodes with comparators in the same mode if the name of the replacement node can be determined – i.e. if it is not nil. ##### Details The patch uses `minetest.after()`. This means there exists a race condition: If the server shuts down during the 0.1 seconds where a comparator is turned on or off, the on/off state of the comparator might be bugged in the future. ##### Testing Steps ###### Verify Bug 1. Enter a world in Minetest 5.4.1 or Minetest 5.5 with Mineclonia commit d9764fdac68190af379266f4c46562e99ecabb4b. 2. Give yourself debug privilege with the command `/grantme debug`. 3. Spawn the comparator test structure using `/spawnstruct test_structure_comparator` **while looking west**. 4. Fill the cauldron using a water bucket (`mcl_buckets:bucket_water`). 5. Place water using a water bucket on the redstone lamp, so that the comparator is flooded. 6. Verify that the Minetest server crashes with a message similar to the following: ``` 2022-02-21 16:22:22: ERROR[Main]: ServerError: AsyncErr: Lua: Runtime error from mod 'mcl_comparators' in callback LuaABM::trigger(): Node name is not set or is not a string! 2022-02-21 16:22:22: ERROR[Main]: stack traceback: 2022-02-21 16:22:22: ERROR[Main]: [C]: in function 'swap_node' 2022-02-21 16:22:22: ERROR[Main]: ...es/Mineclonia/mods/ITEMS/REDSTONE/mcl_observers/init.lua:344: in function 'swap_node' 2022-02-21 16:22:22: ERROR[Main]: .../Mineclonia/mods/ITEMS/REDSTONE/mcl_comparators/init.lua:47: in function 'comparator_activate' 2022-02-21 16:22:22: ERROR[Main]: .../Mineclonia/mods/ITEMS/REDSTONE/mcl_comparators/init.lua:127: in function <.../Mineclonia/mods/ITEMS/REDSTONE/mcl_comparators/init.lua:121> ``` ##### Verify Patch **Prerequisites** 1. Enter a world in Minetest 5.4.1 or Minetest 5.5 with Mineclonia commit be915478afe1a87b1c7846e53971a5961404163c. 2. Give yourself debug privilege with the command `/grantme debug`. 3. Acquire some water buckets (`mcl_buckets:bucket_water`) and empty buckets (`mcl_buckets:bucket_empty`). **Verify that the crash is fixed** 1. Spawn the comparator test structure using `/spawnstruct test_structure_comparator` **while looking west**. 2. Fill the cauldron using a water bucket. 3. Place water on the redstone lamp using a water bucket, so that the comparator is flooded. 4. Verify that the Minetest server does not crash and the comparator drops as an item (`mcl_comparators:comparator_off_comp`). **Verify comparator still works in comparison mode** 1. Spawn the comparator test structure using `/spawnstruct test_structure_comparator`. 2. Fill the cauldron using a water bucket. 3. Verify that the comparator gets turned into a turned-on comparator in comparison mode (`mcl_comparators:comparator_on_comp`) and that a short time later the redstone lamp turns on (`mesecons_lightstone:lightstone_on`). 4. Empty the cauldron using an empty bucket. 5. Verify that the comparator gets turned into a turned-off comparator in comparison mode (`mcl_comparators:comparator_off_comp`) and that a short time later the redstone lamp turns off (`mesecons_lightstone:lightstone_off`) **Verify comparator still works in subtraction mode** 1. Spawn the comparator test structure using `/spawnstruct test_structure_comparator`. 2. Right click the comparator. 3. Verify that the comparator gets turned into a turned-off comparator in subtraction mode (`mcl_comparators:comparator_off_sub`). 4. Fill the cauldron using a water bucket. 5. Verify that the comparator gets turned into a turned-on comparator in subtraction mode (`mcl_comparators:comparator_on_sub`) and that a short time later the redstone lamp turns on. 6. Press the button. Verify that the comparator and the lamp turn off and on again (with a small delay for the lamp). 7. Flip the switch. Verify that the comparator and the lamp turn both off (with a small delay for the lamp). 8. Flip the switch again. Verify that the comparator and the lamp turn on (with a small delay for the lamp). 9. Empty the cauldron using an empty bucket. 10. Verify that the comparator gets turned into a turned-off comparator in subtraction mode and that a short time later the redstone lamp turns off.
erlehmann commented 2022-02-21 18:34:31 +01:00
Author
Owner
Copy Link

This should be tested using a test structure.

This should be tested using a test structure.
erlehmann force-pushed fix-comparator-cauldron-water-crash from 73bb0ff3b0 to 0b779bb273 2022-02-21 18:50:55 +01:00 Compare
erlehmann force-pushed fix-comparator-cauldron-water-crash from 0b779bb273 to e4e12c0b80 2022-02-22 18:16:15 +01:00 Compare
erlehmann commented 2022-02-22 18:39:22 +01:00
Author
Owner
Copy Link

@kabou I have addded a test structure for comparators that you can spawn using the command /spawnstruct test_structure_comparator if you have debug priv. Could you please try to spawn it in commit f092af372faee3b624cca4806d9b701d40591b01 and try to induce a crash, then tell me how to do that? I can not seem to do it. Thank you!

@kabou I have addded a test structure for comparators that you can spawn using the command `/spawnstruct test_structure_comparator` if you have `debug` priv. Could you please try to spawn it in commit f092af372faee3b624cca4806d9b701d40591b01 and try to induce a crash, then tell me how to do that? I can not seem to do it. Thank you!
kabou commented 2022-02-22 22:04:51 +01:00
First-time contributor
Copy Link

Could not get a crash with your test structure.

Made a different test structure.

No crash.

checkout commit from 2 weeks ago.

crash.

checkout your latest commit.

no crash.

Could not get a crash with your test structure. Made a different test structure. No crash. checkout commit from 2 weeks ago. crash. checkout your latest commit. no crash.
screenshot_20220222_215812.png
664 KiB
screenshot_20220222_215817.png
681 KiB
screenshot_20220222_215812.png screenshot_20220222_215817.png
erlehmann commented 2022-02-22 22:15:36 +01:00
Author
Owner
Copy Link

Could not get a crash with your test structure.

Just to be clear, did you try to crash it with my test structure on commit f092af372faee3b624cca4806d9b701d40591b01 or on some other commit?

> Could not get a crash with your test structure. Just to be clear, did you try to crash it with my test structure on commit f092af372faee3b624cca4806d9b701d40591b01 or on some other commit?
kabou commented 2022-02-22 22:21:39 +01:00
First-time contributor
Copy Link

$ git log -1
commit e4e12c0b804eb82784e180210a3316ff9f65ac8f (HEAD -> master)
Author: Nils Dagsson Moskopp nils@dieweltistgarnichtso.net
Date: Mon Feb 21 16:51:52 2022 +0100

$ git log -1 commit e4e12c0b804eb82784e180210a3316ff9f65ac8f (HEAD -> master) Author: Nils Dagsson Moskopp <nils@dieweltistgarnichtso.net> Date: Mon Feb 21 16:51:52 2022 +0100
erlehmann commented 2022-02-22 22:39:32 +01:00
Author
Owner
Copy Link

@kabou ok but can you crash it using my test structure one commit before the fix?

If yes, please tell me where to put the water, I can not figure it out myself.

@kabou ok but can you crash it using my test structure one commit before the fix? If yes, please tell me where to put the water, I can not figure it out myself.
kabou commented 2022-02-22 23:04:15 +01:00
First-time contributor
Copy Link

Like so:

Like so:
screenshot_20220222_230234.png
310 KiB
screenshot_20220222_230527.png
467 KiB
screenshot_20220222_230234.png screenshot_20220222_230527.png
kabou commented 2022-02-22 23:12:46 +01:00
First-time contributor
Copy Link

The inner comparator can also cause a crash like this:

The inner comparator can also cause a crash like this:
screenshot_20220222_230939.png
814 KiB
screenshot_20220222_231016.png
928 KiB
screenshot_20220222_230939.png screenshot_20220222_231016.png
erlehmann force-pushed fix-comparator-cauldron-water-crash from e4e12c0b80 to 715a188c8c 2022-02-22 23:48:50 +01:00 Compare
erlehmann changed title from WIP: Fix redstone comparator flooding crash to Fix redstone comparator flooding crash 2022-02-23 00:25:18 +01:00
erlehmann added the
ready for review
 label 2022-02-23 00:25:49 +01:00
erlehmann changed title from Fix redstone comparator flooding crash to ITEMS/REDSTONE/mcl_comparators: Fix redstone comparator flooding crash 2022-02-23 00:26:18 +01:00
erlehmann force-pushed fix-comparator-cauldron-water-crash from 715a188c8c to be915478af 2022-02-23 00:29:33 +01:00 Compare
erlehmann commented 2022-02-23 14:17:05 +01:00
Author
Owner
Copy Link

It seems that the crash depends on the orientation of the comparator.

I did /spawnstruct test_structure_comparator …

  • 10 times looking north
  • 5 times looking west
  • 5 times looking east
  • 5 times looking south

… and for each structure I filled first the cauldron, then put water on top of the lamp.

It only ever crashed when I was spawning the test structure looking west.

It seems that the crash depends on the orientation of the comparator. I did `/spawnstruct test_structure_comparator` … * 10 times looking north * 5 times looking west * 5 times looking east * 5 times looking south … and for each structure I filled first the cauldron, then put water on top of the lamp. **It only ever crashed when I was spawning the test structure looking west.**
Li0n_2 commented 2022-02-23 22:44:11 +01:00
Member
Copy Link

I tested it, it only crashes if you spawn the structure while looking west

I tested it, it only crashes if you spawn the structure while looking west
cora approved these changes 2022-02-23 23:29:26 +01:00
cora left a comment
Member

All tests worked out, crash is fixed. Tested in 5.4.1 and 5.5 – for some reason the crash still happens in waspsaliva (some 5.4-dev) though. But it's proably not worth investigating.

All tests worked out, crash is fixed. Tested in 5.4.1 and 5.5 – for some reason the crash still happens in waspsaliva (some 5.4-dev) though. But it's proably not worth investigating.
👀 1
cora commented 2022-02-23 23:30:22 +01:00
Member
Copy Link

It only ever crashed when I was spawning the test structure looking west.

I had it crash in all directions but it was in ws i suppose which seems to be more sensitive to this anyways lol.

> **It only ever crashed when I was spawning the test structure looking west.** I had it crash in all directions but it was in ws i suppose which seems to be more sensitive to this anyways lol.
cora merged commit 8810a24ce0 into master 2022-02-23 23:30:47 +01:00
cora deleted branch fix-comparator-cauldron-water-crash 2022-02-23 23:30:48 +01:00
erlehmann removed the
ready for review
 label 2022-02-24 00:05:11 +01: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
4 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#284
No description provided.
Delete Branch "fix-comparator-cauldron-water-crash"

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?