StoneSense crashes on DFHack 0.47.03-beta1

[dfhack]

StoneSense crashes on DFHack 0.47.03-beta1

[lethosor]

If you follow my automated builds link from above, you should get to a page like this (this page may be outdated by the time you click the link, though, so I recommend using the first link instead). Under "Build artifacts", choose the build that matches your DF installation (e.g. "Windows64" for 64-bit Windows DF). Once that file has downloaded, extract "hack/plugins/stonesense.plug.(dll, so, or dylib)" from it and replace the file in your existing hack/plugins folder with it. Generally, when doing this with Stonesense, you should also replace the "stonesense" folder with the downloaded copy, but that shouldn't be necessary in this case. When you start DF, you'll probably see a warning that the plugin was built for a different DFHack build, but as long as it starts with "Warning", it's safe to ignore.

[jeancallisti]

It's a memory corruption from something being loaded improperly as soon as stonesense tries to initialize its assets:

stonesense.plug.dll!std::operator==<ItemConfiguration,std::default_delete>(const std::unique_ptr<ItemConfiguration,std::default_delete> & _Left, void * __formal) Line 2657 C++
stonesense.plug.dll!parseItemElement(TiXmlElement * elemRoot, int basefile) Line 83 C++
stonesense.plug.dll!addSingleItemConfig(TiXmlElement * elemRoot) Line 32 C++
stonesense.plug.dll!ContentLoader::parseItemContent(TiXmlElement * elemRoot) Line 561 C++
stonesense.plug.dll!ContentLoader::parseContentXMLFile(const char * filepath) Line 497 C++
stonesense.plug.dll!ContentLoader::parseContentIndexFile(const char * filepath) Line 431 C++
stonesense.plug.dll!ContentLoader::parseContentIndexFile(const char * filepath) Line 436 C++

the filepath is : stonesense\items\index.txt

EDIT: grei_items.png is the culprit.
At first I thought it might be because of the space in the subfolder's name so I replaced it with underscore, and I also try to open and re-save the PNG as 8-bits colors instead of 32-bits colors.
But neither helped.

[jeancallisti]

I think it crashes because of this :

• In stonesense/items/greiger_items/greiger_items.xml, The XML loader successfully loads the attributes of item "BIN" and identifies that it's main_type 31.
• But then somehow when the game tries to access contentLoader->itemConfigs[main_type] , itemConfigs has a capacity of 0. You can't access cell 31 in an array or vector that has zero cells.

What I don't understand though, is why the array is not enlarged beforehand.
ItemConfiguration seems to be managed in an different way than, for example, CreatureConfiguration, which has a "push" mechanism instead of just accessing the cells by index.

[RosaryMala]

I'm all for using the actual builtin function for this.

[RosaryMala]

Yeah, I'll take a look at the rest of the things and make sure they're all working.

[RosaryMala]

29b430d broke this by removing the part where it makes sure that the item array is the right length.

[RosaryMala]

Also DFhack really should have a ENUM_SIZE macro.

[deadundead]

5b7e774
Can confirm this works.
I've managed to build DFHack, with stonesense checkout to this commit, against a last version of Lazy Mac Pack (v0.47.04).
It still crashes on any attemp to resize the window, but if you don't change it after launching stonesense it seems to run smoothly.
It is also worth mentioning, that I didn't have xcode installed, XML::LibXML installation threw a bunch of errors and XML::LibXSLT didn't install at all. But i was so desperate that I tried the build, and it was OK.

My first time playing DF =D

Also I see that the keyboard control issue is still unsolved, and won't be?
P.S. would be grateful for tips on bulding twbt, as the one included in the Pack refuses to work with latest build of DFHack.

[lethosor]

You need to upgrade to DFHack 0.47.04-r1 (which involves upgrading to DF 0.47.04) - the plugin will only work for that version.

[shawntan]

I'm getting this error on running dfhack in the terminal, then stonesense:

.dwarffortress/dfhack: line 89: 281534 Segmentation fault      (core dumped) setarch "$setarch_arch" -R env LD_PRELOAD="$PRELOAD_LIB" ./libs/Dwarf_Fortress "$@"

The version of dfhack I'm using is DFHack version 0.47.04-beta1 (release) on x86_64, and my system is Linux archlinux 5.6.3-arch1-1 #1 SMP PREEMPT Wed, 08 Apr 2020 07:47:16 +0000 x86_64 GNU/Linux.

The issue seems to be exactly the same as: DFHack/dfhack#1521 (comment)

[shawntan]

Same issue on DFHack version 0.47.04-r1 (release) on x86_64

[lethosor]

Yeah, Stonesense hasn't been meaningfully changed in almost a year. I'm not sure if this issue is due to something that changed in 0.47 that DFHack hasn't caught yet (which would be surprising at this point, but fixable), an issue that needs to be addressed on the Stonesense side (which would be unlikely to happen any time soon unless someone steps up to maintain Stonesense), or something related to newer systems (in which case I would expect at least some more detail in stderr.log).

You wouldn't happen to have a stack trace from this to narrow it down, would you? If you have GDB installed, you could run ./dfhack -g to attach GDB and hopefully generate one when it crashes.

[shawntan]

DFHack/dfhack#1521 (comment) also reaches the same conclusion.

[RosaryMala]

I'll try to get this fixed.

[jeancallisti]

Not sure you're quoting the correct commit, as the one you quoted mentions TerrainMaterialConfiguration, whereas this issue is with ItemConfiguration ? (Even though the logic does seem similar and does seem to have been broken)

[RosaryMala]

Oh, he changed a bunch of things, but I realized that his specific chances weren't what was causing it. Still trying to make sure I caught everything, 5b7e774 may fix it.

[jeancallisti]

Your change does two things :

1. You changed the way the resizing is done

• the way it was done before : if the vector is too small, clear it entirely then push null into the vector until it reaches the required size.
• The way you're doing it : if the vector is too small, use .resize to enlarge it, then put null into every cell.

This first change is not really necessary. EDIT: It is necessary

2. You change the size of the array to ENUM_LAST_ITEM(item_type) +1 instead of ENUM_LAST_ITEM(item_type)

I think that's the solution to our issue. Before, if we encountered an item type which enum was going up to, let's say, 31, then we would resize the array to 31 instead of 32. Which means we could never go up to the cell at index 31! That was clearly a bug that needed to be fixed.

I shall end with a sour comment about how once again when looking at C++ code, there's a fatal bug in a simple operation because someone thought they were too smart to just use off-the-shelf, one-liner array primitives... and made a mistake in their custom 10-lines replacement. ;)

[jeancallisti]

I've tested it. It works.
Contrary to what I said, both changes are needed. And I cant explain why this works...

if (config.size() != (ENUM_LAST_ITEM(item_type) + 1))
    config.resize(ENUM_LAST_ITEM(item_type) + 1);
for (size_t i = 0; i < config.size(); i++)
{
    config[i] = nullptr;
}

...while this crashes :

uint32_t currentsize = (uint32_t)config.size();

if (currentsize < ENUM_LAST_ITEM(item_type) +1) {
    currentsize = ENUM_LAST_ITEM(item_type) +1;
}
config.clear();
while (currentsize < config.size()) {
    config.push_back(nullptr);

[RosaryMala]

This is why I hate C++.

[lethosor]

config = vector<whatever_type>(ENUM_LAST_ITEM(item_type) + 1, nullptr) would probably be less error-prone than either of those

[jeancallisti]

Ha ha I love how the hardened C++ dev answers "this would be less error-prone" rather than "look, those 10 lines are now one, I used a built-in function".

[lethosor]

I mean, re-implementing built-in operations usually is more error-prone regardless of the language. I'm more inclined to blame the logic here rather than the language itself.

Looking again at the two snippets you posted, the main difference I'm seeing is that the first one (which you said works) resizes config to be exactly the desired size, while the second one resizes it to be at least the desired size - if it was larger before, it will still be larger. Just by looking at it, I don't see any way that it can crash by itself, but it could probably affect other parts of Stonesense in difficult-to-predict ways. (I would have guessed that the first snippet would be more likely to result in a crash later on, but it's hard to say without more context.)

Casting currentsize from a size_t to a uint32_t (which is sometimes smaller) is inadvisable, but probably unlikely to trigger any issues for smaller vectors.

Edit: oh, I think I see the issue in the second snippet - currentsize is the desired size of the vector (it's a misleading name IMO), so if it's set to e.g. 5, it will never be smaller than config.size() after config.clear() is called (5 < 0 is false, so the body of the while loop will never run).

[jeancallisti]

Ah yes, indeed, it should be while(config.size()<currentSize ) , not while(currentsize < config.size()).
Mystery solved!

But that can be dumped altogether if the one-liner is used. So yes, I vote for that.

@RosaryMala not to be lazy but can you check that this way of doing things is not used in other XXXConfiguration.cpp files?

[lethosor]

We would probably have to move Stonesense to a standalone process to make keyboard input work (that would have other benefits, like crashes not bringing down DF, but would also need someone familiar with Stonesense and willing to put in the effort).

For reference, I'm pretty sure that the automated builds from https://dfhack.org/builds/ (like this one) include Stonesense.

[DanielLaberge]

Until this is fixed, could someone provide a brief walkthrough/workaround on how to get this working with say, the LNP? It would be much appreciated <3

[DanielLaberge]

Perfect, thank you. I can confirm this works with PeridexisErrant's Starter Pack 0.47.04-r06.

[lc-thomas]

Doesn't work for me :/

[DFHack]# load stonesense
Plugin stonesense was not built for this version of DFHack.
Plugin: 0.47.04-r1, DFHack: 0.47.03-beta1

[lethosor]

Assuming 5b7e774 fixed this, this should be addressed in 0.47.04-r2. Feel free to open a new issue if problems continue.