Fix crash when blowing up an unknown node #38

ryvnf · 2021-04-22T12:51:46+02:00

ryvnf commented

2021-04-22 12:51:46 +02:00

The problem is that unknown nodes do not have a blast resistance which leads to a crash. This solves that by giving them a blast resistance of zero.

I think it can be discussed how unknown nodes should behave when it comes to explosions.

One alternative would be to make unknown nodes have a blast resistance of zero but not be removed after an explosion. But I do not think making a more complicated solution just to handle unknown nodes differently from other nodes would be the justified. This is because they should be very rare and only occur after errors when updating mods. I think just assigning them a blast resistance (either zero or indestructible) is the best solution.

Overall I think giving them a blast resistance of zero makes sense since that is the default value for all other nodes without a defined blast resistance. This makes them somewhat vulnerable to creeper explosions though.

The problem is that unknown nodes do not have a blast resistance which leads to a crash. This solves that by giving them a blast resistance of zero. I think it can be discussed how unknown nodes should behave when it comes to explosions. One alternative would be to make unknown nodes have a blast resistance of zero but not be removed after an explosion. But I do not think making a more complicated solution just to handle unknown nodes differently from other nodes would be the justified. This is because they should be very rare and only occur after errors when updating mods. I think just assigning them a blast resistance (either zero or indestructible) is the best solution. Overall I think giving them a blast resistance of zero makes sense since that is the default value for all other nodes without a defined blast resistance. This makes them somewhat vulnerable to creeper explosions though.

n_to commented

2021-04-23 12:15:16 +02:00

@ryvnf: Do you have any suggestions how I could test this? I found now way to create an unknown node via commands so I've no clue how I could provoke the crash.

ryvnf commented

2021-04-23 13:26:12 +02:00

minetest.register_node("unknown:unknown", {
	description = "Unknown Block",
	_doc_items_longdesc = "Unknown Block",
	_doc_items_hidden = false,
	tiles = {"default_stone.png"},
	is_ground_content = true,
	stack_max = 64,
	groups = {pickaxey=1, stone=1, building_block=1, material_stone=1},
	drop = 'mcl_core:cobble',
	_mcl_blast_resistance = 6,
	_mcl_hardness = 667,
	_mcl_silk_touch_drop = true,
})

This is what I used for testing. Place it in the mods/unknown/init.lua and enable it for the testing world. It should then show up in the creative inventory. Place one or more node of it in the world. Then log out and disable the mod to make it become an unknown node.

```lua minetest.register_node("unknown:unknown", { description = "Unknown Block", _doc_items_longdesc = "Unknown Block", _doc_items_hidden = false, tiles = {"default_stone.png"}, is_ground_content = true, stack_max = 64, groups = {pickaxey=1, stone=1, building_block=1, material_stone=1}, drop = 'mcl_core:cobble', _mcl_blast_resistance = 6, _mcl_hardness = 667, _mcl_silk_touch_drop = true, }) ``` This is what I used for testing. Place it in the `mods/unknown/init.lua` and enable it for the testing world. It should then show up in the creative inventory. Place one or more node of it in the world. Then log out and disable the mod to make it become an unknown node.

e commented

2021-04-23 13:56:12 +02:00

@ryvnf Would that snippet actually cause the crash if blown up? It looks like it specifies _mcl_blast_resistance in the item spec.

@ryvnf Would that snippet actually cause the crash if blown up? It looks like it specifies `_mcl_blast_resistance` in the item spec.

ryvnf commented

2021-04-23 14:20:55 +02:00

@e That was for testing my enchanting changes. It will only crash if it actually becomes an unknown node and for that to happen the mod needs to be disabled after placing the node.

erlehmann commented

2021-04-23 15:20:25 +02:00

I think just assigning them a blast resistance (either zero or indestructible) is the best solution.

I agree. i guess any more complicated solution would not pass the "solution fits in head" test.

Overall I think giving them a blast resistance of zero makes sense since that is the default value for all other nodes without a defined blast resistance.

I agree, “can be blown up” is the default and it is what I would expect, too.

> I think just assigning them a blast resistance (either zero or indestructible) is the best solution. I agree. i guess any more complicated solution would not pass the "solution fits in head" test. > Overall I think giving them a blast resistance of zero makes sense since that is the default value for all other nodes without a defined blast resistance. I agree, “can be blown up” is the default and it is what I would expect, too.

erlehmann requested changes 2021-04-23 15:28:36 +02:00

erlehmann left a comment

I would appreciate a comment in the code that this prevents crashing on unknown nodes, because otherwise to future developers it might look like some bug fix that might not be necessary anymore.

ryvnf force-pushed mcl_explosions from 39216ded3a to 31341ca57b

2021-04-23 16:40:15 +02:00

Compare

ryvnf commented

2021-04-23 16:41:58 +02:00

Made a comment to clarify that the or 0 is to set the blast resistance of unknown nodes to 0.

Made a comment to clarify that the `or 0` is to set the blast resistance of unknown nodes to 0.

ryvnf requested review from erlehmann 2021-04-23 16:42:03 +02:00

n_to commented

2021-04-23 18:02:49 +02:00

Test successfull. I created an unknown block according to the instructions @ryvnf gave.

In master-branch I could successfully crash the game by exploding TNT next to the unknown block
In the PR-branch no crash could be created

Please find attached the debug-log for the crash.

**Test successfull.** I created an unknown block according to the instructions @ryvnf gave. * In master-branch I could successfully crash the game by exploding TNT next to the unknown block * In the PR-branch no crash could be created Please find attached the debug-log for the crash.

unknown_crash.txt

1.5 KiB

n_to approved these changes 2021-04-23 18:04:47 +02:00

n_to left a comment

Fix successful

Crash could be reproduced with unpatched version.
Crash could not be reproduced with patched version

Details and debug.log excerpt for crash can be found here: #38 (comment)

Fix successful * Crash could be reproduced with unpatched version. * Crash could not be reproduced with patched version Details and debug.log excerpt for crash can be found here: https://git.minetest.land/Mineclonia/Mineclonia/issues/38#issuecomment-21174

erlehmann commented

2021-04-23 18:39:21 +02:00

The comment is good. Let me double-check that I can reproduce what @n_to did really quick, maybe I notice something else (probably not though).

erlehmann approved these changes 2021-04-24 05:28:51 +02:00

erlehmann left a comment

I tested this using the pontoons mod for unknown nodes.

It crashed when exploding several unknown pontoon nodes using TNT without this patch.

It did not crash when exploding several unknown pontoon nodes using TNT with this patch.

I tested this using the pontoons mod for unknown nodes. It crashed when exploding several unknown pontoon nodes using TNT without this patch. It did not crash when exploding several unknown pontoon nodes using TNT with this patch.

erlehmann merged commit d6463fe29a into master

2021-04-24 05:29:10 +02:00

erlehmann referenced this issue from a commit

2021-04-24 05:29:11 +02:00

Merge pull request 'Fix crash when blowing up an unknown node' (#38) from mcl_explosions into master

erlehmann referenced this pull request

2021-04-24 05:34:57 +02:00

Crash when blowing up an unknown node #42

erlehmann referenced this pull request

2021-04-24 05:35:11 +02:00

Crash when blowing up an unknown node #42

Li0n added the