More TFork segfault problems

[Lord Estraven]

Make a wish for something that isn't a valid item or monster (e.g. "foo") and you'll get a Lua error about get_stick_charge indexing a nil value, followed by a crash. Backtrace is as follows.

Code: Select all

#0  object_desc_str (t=0xbffff054 "", s=0x0) at object1.c:1262
#1  0x0810c54e in object_desc (buf=0xbffff178 "wand of poison blood", 
    o_ptr=0xbffff2b0, pref=0, mode=0) at object1.c:2059
#2  0x08136a2a in test_object_wish (name=0xbffff330 "foo", o_ptr=0xbffff2b0, 
    forge=0xbffff2b0, what=0x81ea03f "wish") at xtra2.c:7411
#3  0x08136dde in make_wish () at xtra2.c:7570
#4  0x081c04aa in do_cmd_debug () at wizard2.c:1912
#5  0x080ec59e in process_command () at dungeon.c:3558
#6  0x080f159e in process_player () at dungeon.c:4875
#7  0x080f2296 in dungeon (new_game=<value optimized out>) at dungeon.c:5327
#8  play_game (new_game=<value optimized out>) at dungeon.c:5816
#9  0x081dea5c in main (argc=1, argv=<value optimized out>) at main.c:908

Sticking a check for NULL in object1.c fixes the crash, but still produces a Lua error.

The strange thing is, I haven't made any changes at all to the make_wish() code or anything related. This only appeared after I modified some of the school spells. So no real idea what's going on.

[AnonymousHero]

Well, object1.c line 2059 is trying to copy the school spell name into the object description string, so you've probably messed up the school_spells[] array. Either that or the o_ptr->pval2 is out of bounds.

If your changes to school_spells[] have been happening over multiple commits, you can try to narrow down which exact commit caused it(*) by using "git bisect".

(*) Assuming you can reproduce the problem reliably.

[Lord Estraven]

Hmm. Could one mess up the school_spells[] array by changing the names of too many spells, without changing the number of them? Could something bad happen if some of the names are too long?

(I've run into huge trouble before by removing one of the spells, but I've been avoiding doing that.)

[AnonymousHero]

If you have extremely long names that could be an issue. The full descriptions of all possible items has to fit into various temporary buffers, and the buffers might be too short.

If you didn't change the number of spells, then that's probably not the problem. However, it might be that some parts of school_spells[] are not being initialized properly after you've changed the names (just guessing, but it's possible).

[Lord Estraven]

Hmm. The problem seemed to appear after I changed "Elemental Shield" to "Mana Storm". Can't quite pin it down though.

[Lord Estraven]

Okay I threw Valgrind at it... Here's what it says.

Code: Select all

==11061== Invalid read of size 4
==11061==    at 0x810C505: object_desc (object1.c:2059)
==11061==    by 0x81369C9: test_object_wish (xtra2.c:7411)
==11061==    by 0x8136D7D: make_wish (xtra2.c:7570)
==11061==    by 0x81C0429: do_cmd_debug (wizard2.c:1912)
==11061==    by 0x80EC55D: process_command (dungeon.c:3558)
==11061==    by 0x80F155D: process_player (dungeon.c:4875)
==11061==    by 0x80F2255: play_game (dungeon.c:5327)
==11061==    by 0x81DE9DB: main (main.c:908)
==11061==  Address 0x4602724 is 12 bytes before a block of size 1,440 alloc'd
==11061==    at 0x4024918: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)

I'm not sure, but it looks to me like it's saying that memory is being allocated that's already in use, or something like that.

Also as per T1 I get a ton of

Code: Select all

Conditional jump or move depends on uninitialised value(s)

How DG used to maintain this thing I have no idea.

[Lord Estraven]

Haha I LOVE git bisect.

Surprisingly (or maybe not?) it's looking like this bug was actually introduced when I switched from CMake back to makefiles. Go figure.

BTW is it just me or does the game compile faster using CMake?

[Lord Estraven]

... No that's not it either. The bad commit was the one I thought it was, which means the problem definitely started with renaming spells.

This is the weirdest bug I've ever seen!

[Lord Estraven]

Got it... It was the Thunderstorm -> Thunderbolt change. I'll have to figure how what exactly was going on there.

[Lord Estraven]

Okay, I've found the answer and I don't like it: some spells apparently can't be changed or removed lest a crash occur. Thunderstorm is one of those spells. This sucks, because Thunderstorm happens to be quite useless.

I'm going to see if there's something about it hardcoded...

[Lord Estraven]

... Nothing I can find, but it seems I can't touch any spell in the Air school without causing crashes. For now I guess I'll just leave them alone.