ToME 2 maintenance

[Lord Estraven]

Yeah, I figured that out, sorry. Looks like something you deprecated.

[AnonymousHero]

Yeah, I figured that out, sorry. Looks like something you deprecated.

No worries. Btw, in my experience sometimes "deliberately crash the program" is actually the best thing you can do... but it's not something I "reach for" often .

EDIT: It's basically always better to "abort" rather than run into the dreaded Undefined Behavior.

[Lord Estraven]

Good point.

Meanwhile, Linux SSP is actually getting invoked no matter what one enters on the Automizer prompt. Hmm.

[Lord Estraven]

The buffer overrun here, wherever it is, seems to have been present at least since the introduction of the new jansson-based Automizer. Maybe earlier.

Edit: right, git bisect. D'oh!

Edit: yeah, 2.3.9-ah works. Let's start bisecting.

[Lord Estraven]

Alright, the first bad commit is 6add91e17080e06cae938a31c53c94e59c7f0bfb.

This commit basically contains the entirety of the jansson-based automizer code, almost as it appears today.

[Lord Estraven]

So, working on the negation rule crash, this is interesting:

Code: Select all

[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
tome: /home/proteus/Projects/tome2/src/squeltch.c:2083: display_condition: Assertion `condition != ((void *)0)' failed.

Program received signal SIGABRT, Aborted.
0xb70c59dc in raise () from /lib/libc.so.6
(gdb) bt
#0  0xb70c59dc in raise () from /lib/libc.so.6
#1  0xb70c71f3 in abort () from /lib/libc.so.6
#2  0xb70beac6 in __assert_fail_base () from /lib/libc.so.6
#3  0xb70beb77 in __assert_fail () from /lib/libc.so.6
#4  0x080d44cb in display_condition ()
#5  0x080d4a9f in display_condition ()
#6  0x080d52f9 in display_rule ()
#7  0x080d5cbc in do_cmd_automatizer ()
#8  0x0816ee06 in do_cmd_options ()
#9  0x0809c9f2 in process_command ()
#10 0x0809d25e in process_player ()
#11 0x0809ddd5 in dungeon ()
#12 0x0809e7f2 in play_game ()
#13 0x081de7a0 in main ()
(gdb) 

The assert that's breaking looks like this

Code: Select all

static void display_condition(condition_type *condition)
{
        byte bcol = TERM_L_GREEN;
        byte ecol = TERM_GREEN;
        int i;
               
        assert(condition != NULL);

and this is the caller:

Code: Select all

static void display_rule(arule_type *rule)
{
        cptr action_s;
        int hgt, wid;

        action_s = action_to_string(rule->action);

... but here is how it calls display_condition():

Code: Select all

        /* Write out the condition */
        if (rule->condition != NULL)
        {
                display_condition(rule->condition);
        }

... It's checking that rule->condition is not NULL. And only displaying it if it's not NULL. But it's NULL anyway.



Maybe something to do with what the NULL macro actually represents on different platforms?

[Lord Estraven]

I added another assert before the 'if' statement, and it's obviously a null pointer before the 'if' is reached. Hmm.

[Lord Estraven]

Wait wait wait! display_condition is recursive, so it must be nullifying the pointer somewhere. Just a minute.

[Lord Estraven]

Okay, I think I got this. The M_NOT case in display_condition() is invoking the function recursively on condition->subcondition, which is explicitly NULL. (Line 2219 in squeltch.c.) Let's see what happens when we check for that being NULL too.

Edit: That works. I'll look over it for further invocations without NULL checking, then request a merge. (With apologies for the last merge request.)

[Lord Estraven]

Okay, so for the SSP crash, the problem is this

Code: Select all

static int create_new_rule()
{
        action_type action;
        char name[20] = { '\0' };

The name[] buffer is too small for some reason. Not sure exactly what's happening here though - "No name" is only 8 characters (including the terminating NUL).

Setting the buffer size to 100 works (obviously?), but that doesn't tell me why things are breaking, and might break again in the future.

I also tried using snprintf. Oddly, that resulted in exactly the same crash.

Weird.

Edit: looks like the buffer size must be > 80 characters to not cause an overflow. Hmm.

Edit 2: ah got it, it's an off-by-one error.

Code: Select all

        if (!input_box("Name?", hgt / 2, wid / 2, name, sizeof(name)+1))

That extra '+1' breaks things.

[Lord Estraven]

Okay - all three of those are in my automizer-fix branch, and I've filed a merge request.

[Lord Estraven]

Hey @AnonymousHero, did you change something in the cpp branch before merging it? Because now the compile fails on cmd1.cc.

Code: Select all

[  3%] Building CXX object src/CMakeFiles/game.dir/cmd1.cc.o
/home/proteus/Games/tome2/src/cmd1.cc: In function ‘bool_ pattern_seq(int, int, int, int)’:
/home/proteus/Games/tome2/src/cmd1.cc:2848:30: error: cannot pass objects of non-trivially-copyable type ‘struct cave_type’ through ‘...’
                 cave[c_y][c_x]);
                              ^
make[2]: *** [src/CMakeFiles/game.dir/cmd1.cc.o] Error 1
make[1]: *** [src/CMakeFiles/game.dir/all] Error 2
make: *** [all] Error 2

I will see if I can fix this. But looking at the error, I suspect it will require a function prototype to be changed.

[AnonymousHero]

Hey @AnonymousHero, did you change something in the cpp branch before merging it? Because now the compile fails on cmd1.cc.

Code: Select all

[  3%] Building CXX object src/CMakeFiles/game.dir/cmd1.cc.o
/home/proteus/Games/tome2/src/cmd1.cc: In function ‘bool_ pattern_seq(int, int, int, int)’:
/home/proteus/Games/tome2/src/cmd1.cc:2848:30: error: cannot pass objects of non-trivially-copyable type ‘struct cave_type’ through ‘...’
                 cave[c_y][c_x]);
                              ^
make[2]: *** [src/CMakeFiles/game.dir/cmd1.cc.o] Error 1
make[1]: *** [src/CMakeFiles/game.dir/all] Error 2
make: *** [all] Error 2

I will see if I can fix this. But looking at the error, I suspect it will require a function prototype to be changed.

I have a fix already, but I may have forgotten to push it -- will investigate ASAP. (You're compiling with Clang, right?)

I believe it should work to compile with gcc.

[Lord Estraven]

This was with GCC (4.8.2).

BTW this is stuff related to the Straight Road (i.e. Zangband Pattern). Straight Road vaults don't even exist any more. It can probably be removed.

Edit: whoops, and that line is for Wizard Mode anyway.

Edit 2: I just modified the printf invocation to not provide any coordinates (i.e. no %d).

Though, I'm gonna take a look at the vaults info file. I want to see if I can reintroduce Straight Road vaults.

[AnonymousHero]

This was with GCC (4.8.2).

BTW this is stuff related to the Straight Road (i.e. Zangband Pattern). Straight Road vaults don't even exist any more. It can probably be removed.

Oh, perhaps I got clang/gcc mixed up. Anyway, the fix is pushed now.