Two Questions: a) Code Still Supported? b) Crashing somewhere in show_score()

Posted by Parsival on Wed 04 Feb 2009 02:27 AM — 16 posts, 76,048 views.

Parsival Wed 04 Feb 2009 02:27 AM #0

Hi, and thanks for reading my post!

First question: As I peruse the dawnoftime.org site and try (unsuccessfully) to activate an account on the forums there, I notice that there hasn't been much activity surrounding this codebase on the forums for quite some time... Has this project died, or is someone still working it?

It's a pretty hot codebase, and I'm anxious to learn more about it!

Second question: I'm running Ubuntu 8.10, and was successful in getting the source to compile and get the server running. I've created a couple characters, one of which is now the administrator.

When I type 'score', the mud crashes immediately.

I'm still a little unfamiliar with gdb, and don't know how to load up a background process in order to set breakpoints in the code, so I am unsure where the buffer overrun is happening. If you might have a moment to look through this backtrace and give me some suggestions on how to proceed, I'd be very grateful.

Backtrace:


#0  0xb7f49430 in __kernel_vsyscall ()
#1  0xb7cae880 in raise () from /lib/tls/i686/cmov/libc.so.6
#2  0xb7cb0248 in abort () from /lib/tls/i686/cmov/libc.so.6
#3  0xb7cec10d in ?? () from /lib/tls/i686/cmov/libc.so.6
#4  0xb7d7d548 in __fortify_fail () from /lib/tls/i686/cmov/libc.so.6
#5  0xb7d7b670 in __chk_fail () from /lib/tls/i686/cmov/libc.so.6
#6  0xb7d7ad68 in ?? () from /lib/tls/i686/cmov/libc.so.6
#7  0xb7cf0a18 in _IO_default_xsputn () from /lib/tls/i686/cmov/libc.so.6
#8  0xb7cc3083 in vfprintf () from /lib/tls/i686/cmov/libc.so.6
#9  0xb7d7ae14 in __vsprintf_chk () from /lib/tls/i686/cmov/libc.so.6
#10 0xb7d7ad5d in __sprintf_chk () from /lib/tls/i686/cmov/libc.so.6
#11 0x080fda0e in get_ictimediff (t1=1196692920, t2=1233712584, 
    icyears_added_to_result=0) at /usr/include/bits/stdio2.h:35
#12 0x0819b5ac in show_score (ch=0xb74e83d0, v=0xb74e83d0) at score.cpp:126
#13 0x0819c4f8 in do_score (ch=0xb74e83d0, argument=0x83daa05 "")
    at score.cpp:284
#14 0x08100eae in interpret (ch=0xb74e83d0, argument=0x83daa05 "")
    at interp.cpp:1322
#15 0x080afc84 in process_input (c=0xb74ea1fc) at comm.cpp:3875
#16 0x08156c9b in netio_process_input_from_polled_connections ()
    at netio.cpp:1259
#17 0x080b460d in game_loop () at comm.cpp:1111
#18 0x080b495c in main (argc=1, argv=0xbf848e44) at comm.cpp:743

Nick Gammon Australia Forum Administrator Wed 04 Feb 2009 03:44 AM #1

I haven't seen much activity for quite a while. The last I heard was that the developer had gone to work overseas.

Can you tell me specifically which version of DoT you downloaded (ie. file name)? I can try to reproduce the crash.

Parsival Wed 04 Feb 2009 12:28 PM #2

I sure can:

dawn1.69r-src.tgz and dawn1.69r-support.tgz, located here:

http://www.dawnoftime.org/download/

I had to do two things to get it to compile:
Add -Wno-write-strings to C_FLAGS in ~/dot/src/configure/makefile.in, and I had to remove "lockers_object::" from line 81 of lockers.h.

This is the thread where I found that information:
http://forums.dawnoftime.org/viewtopic.php?t=2228

Also, I added -g3 to C_FLAGS because I read somewhere (perhaps in your article on the subject?) that this makes gdb more functional when debugging.

Finally, I ran ~/dot/src/configure/configure to allow the script to discover my setup and build a makefile, then in /src I ran a make.

It compiled fine (aside from some warnings), and I was able to boot the server. I created a player, logged him off, moved his file to ~/dot/player/immortal/, changed his level manually to 100, and relogged him. I saved the character, then typed 'score' and the mud crashed.

With any mortal character, the same thing happens. I tried to run the program from gdb, but because it forces itself into the background, the breakpoint I set at do_score() didn't fire.

Thanks for looking at this, Nick, and thanks for being so responsive to those of us who are less experienced. It's a rare thing, these days, to see someone help others just for the sake of helping others, and not for personal gain. Your kindness is noted and very much appreciated!

-Steve-o

Edit: Oh, if you could point me in the right direction as to how to debug a running background process in gdb, I'd love to know more about that, too!

Nick Gammon Australia Forum Administrator Thu 05 Feb 2009 02:29 AM #3

Well this is an interesting challenge. I can't get it to crash consistently (itself a bad thing) but have certainly had a few crashes.

Quote:

if you could point me in the right direction as to how to debug a running background process in gdb

Two methods can be used. The first is when the MUD starts it says something like:


:: Mud running in background, process id = 18439.

Now just type in:


gdb dawn 18439

That attaches it to that process.

Alternatively, to save mucking around with the background process in this case, start it like this:


gdb dawn
run -nb

The -nb flags tells DoT to not run in the background. Then you can debug normally, set breakpoints etc.

I see from the compile I get this warning:


g++ -c -Wall -g -O  -g3 who.cpp -o obj/who.o
who.cpp: In function ‘void do_who(char_data*, char*)’:
who.cpp:594: warning: operation on ‘wch’ may be undefined

Operating on undefined variables is usually bad, especially if they are pointers.

That line in the code is:


for ( wch = first_on_who; wch; wch = wch = levelsort?wch->next_who_list:wch->next_player )

The expression: wch = wch

... seems to me to not do anything useful, this may be the undefined variable part.

I am pretty sure I have used this version of Dawn before without problems. Probably the C compiler has been tightened up, and obscure coding techniques are now causing problems.

With my guidance for running gdb on the operating code, perhaps you can narrow down the problem. I had mine crash on do_who, not do_score.

Nick Gammon Australia Forum Administrator Thu 05 Feb 2009 02:31 AM #4

I changed that line to:


for ( wch = first_on_who; wch; wch = levelsort?wch->next_who_list:wch->next_player )

In other words, I got rid of the redundant assignment. It didn't crash next time, but then it didn't crash every time anyway, so I'm not sure that is really the problem.

Parsival Thu 05 Feb 2009 11:47 PM #5

Thanks! I corrected that bug, hopefully avoiding a future crash.

After your help with gdb, I was able to set some breakpoints and step through the code, and I'm confident I've found the function that is causing the problem.

In ictime.cpp, the function get_ictimediff() is called from show_score(), which is called from do_score().

This function is also called from do_rpsheet(), and when I type that command, the mud crashes in exactly the same way.

Here is the code for that function. From what I can understand, this function returns a string containing the age of the character in years, days, months and seconds, taking into account the current amount of time played and the starting age of a character (17 years).

It's been a -long- time, Nick, since I studied C++, and in my current vocation, I have no need for it at all, so my memory is fuzzy on the algorithm for deriving time from a number of seconds...but if I'm reading this right, the %= operator is used to distil the age of the character in seconds ( abs((int)(t1-t2)) ) down to years, days, months and seconds.

What I don't understand is the line:

++index%=3;

What is the purpose of this? I did a 'print index' prior to this line, which indicated 0, and afterward it contained 1.
Why not just index=1;? It's a local integer used in an array later...

Anyway the line where the mud crashes is the sprintf statement. I'm really fuzzy on multidimensional array syntax these days, but it seems strange to me how timebuf[] is created and used in this function.

Here is the function in question:

char *get_ictimediff(time_t t1, time_t t2, int icyears_added_to_result)
{
    static char timebuf[MSL][3];
    static int index; 
    int dsec, icyear, icmonth, icweek, icday;

    ++index%=3;

    // initialise working string
    timebuf[index][0]=0;

    dsec   = abs((int)(t1-t2)); // number of IRL seconds between the 2 times
	icyear = dsec/ICTIME_IRLSECS_PER_YEAR;
	icyear+= icyears_added_to_result;
	dsec  %= ICTIME_IRLSECS_PER_YEAR;
    icmonth= dsec/ICTIME_IRLSECS_PER_MONTH;
	dsec  %= ICTIME_IRLSECS_PER_MONTH;
    icweek = dsec/ICTIME_IRLSECS_PER_WEEK;
	dsec  %= ICTIME_IRLSECS_PER_WEEK;
	icday  = dsec/ICTIME_IRLSECS_PER_DAY;	

    sprintf(timebuf[index],"%d year%s, %d month%s, %d week%s, %d day%s",
		icyear, 
		(icyear!=1?"s":""),
		icmonth, 
		(icmonth!=1?"s":""),
		icweek, 
		(icweek!=1?"s":""),
		icday, 
		(icday!=1?"s":""));

    return(timebuf[index]);
}

Thanks again for your time, Nick. I promise to dust off those old C and C++ books and make you proud one day....

S

Nick Gammon Australia Forum Administrator Fri 06 Feb 2009 02:50 AM #6

Quote:

What I don't understand is the line:

++index%=3;

What is the purpose of this? I did a 'print index' prior to this line, which indicated 0, and afterward it contained 1.
Why not just index=1;? It's a local integer used in an array later...

Well for one thing it is declared static, which means its value is preserved between calls.

What that line is doing is:

Add 1 to index
Take the modulus of that new value, modulo 3.
Store the result back into index.

Why, I am not sure.

Anyway, I can't help thinking the declaration is backwards. Take this:


static char timebuf[MSL][3];

MSL is declared in params.h to be 8192. So it is using a big buffer, and 3 of them (one for each possible index, 0, 1 and 2).

But it is being used the other way around, eg.


// initialise working string
    timebuf[index][0]=0;

Offhand I would try this:


static char timebuf[3][MSL];

Then at least the order in the declaration agrees with the usage.

Parsival Fri 06 Feb 2009 12:22 PM #7

At your suggestion, in get_ictime() located in ictime.cpp, I changed the declaration of the array to read:


 static char timebuf[3][MSL];

It worked perfectly, and pointed out very clearly that I need to brush up on syntax after so many years of inactivity.

By the way, I think I may understand the purpose of that line

++index%=3;

now.

Using the modulus operator in this way causes index to cycle through values of 0, 1, and 2, then starting again at 0.

As I think about why, I'm wondering if perhaps this is done to reuse the memory allocated to timebuf[][] repeatedly rather than reallocating every time the function is called?

In other words, the string is written to timebuf[0][...] the first time, then timebuf[1][...] the second, and timebuf[2][...] the third. Then it starts with timebuf[0][...] again, overwriting the old information stored there.

Players type 'score' quite often, I'd expect, so this may be a little faster than reallocating memory every time this function is called? Am I off base?

If this -is- the case, would a defined macro be even faster and more efficient for this process?

Thanks a million, Nick!

-S

Nick Gammon Australia Forum Administrator Fri 06 Feb 2009 07:42 PM #8

It is an ingenious suggestion, but the timebuf buffer is statically allocated fixed-length buffer. Thus, it is never reallocated at runtime.

I initially thought that maybe he used 3 indexes so he could compare times (eg. this time compared to last time) but I can't see anywhere that is done.

Quote:

Your kindness is noted and very much appreciated!

Thanks for that. :)

ThomasWatts USA Sat 07 Feb 2009 08:33 AM #9

I hate to barge into this so late in the game, but


for ( wch = first_on_who; wch; wch = levelsort?wch->next_who_list:wch->next_player )

is missing its parenthesis, which is why 'wch = wch' seemed redundant.
Should be


for ( wch = first_on_who; wch; wch = (wch = levelsort ? wch->next_who_list : wch->next_player) )

As for the 'timebuf', I'm not sure why there is three. Each time the function is called the string is changed. Nothing outside the function can use 'timebuf' either.
8192 is also going to be a waste of memory. 1024 would be more than enough string space.

Nick Gammon Australia Forum Administrator Sat 07 Feb 2009 09:44 PM #10

It still seems redundant to me.


(wch = levelsort ? wch->next_who_list : wch->next_player)

... establishes a new value for wch. Then assigning that to wch doesn't seem to me to achieve much.

ThomasWatts USA Sun 08 Feb 2009 06:26 AM #11

Yes, perhaps it should be wch == levelsort
Check for comparison then assign.

David Haley USA Mon 09 Feb 2009 01:16 PM #12

It depends on the precedence -- if the assignment is evaluated as the expression test, that is different from assigning levelsort ? x : y to wch. I.e.,

(wch = levelsort ? wch->next_who_list : wch->next_player)

is not the same as

wch = (levelsort ? wch->next_who_list : wch->next_player)

Because in the latter, wch gets the value from the conditional, whereas in the former, wch just gets levelsort and the whole expression evaluates to the value from the conditional.

However,

wch = (wch = levelsort ? wch->next_who_list : wch->next_player)

is indeed the same thing as

wch = (levelsort ? wch->next_who_list : wch->next_player)

as far as the final value of wch is concerned.

Nick Gammon Australia Forum Administrator Mon 09 Feb 2009 06:45 PM #13

Quote:

Because in the latter, wch gets the value from the conditional, whereas in the former, wch just gets levelsort and the whole expression evaluates to the value from the conditional.

I cannot agree with you there, David. You are right that the overall behaviour depends on the operator precedence, but the C++ operator precedence is well defined, with the ternary condition having a higher precedence than direct assignment. See, for example:

http://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B

There are some subtleties, using the ternary operator, if you also use = inside the expression (after the ?) however that does not apply in this case.

This test program illustrates it.


#include <iostream>

int main (void)
  {
  int a, b;
  int c = 1;

  a = (b = c ? 4 : 5);

  std::cout << "a = " << a << std::endl;
  std::cout << "b = " << b << std::endl;
  std::cout << "c = " << c << std::endl;

  return 0;
  }


Output

a = 4
b = 4
c = 1

You will note that both a and b are the value 4, whereas you predicted that b would take on the value of c (which is 1).

David Haley USA Mon 09 Feb 2009 07:00 PM #14

Well I did say that it depended on the precedence (which I couldn't recall at the time), and that I was assuming that the assignment was evaluated as the expression test :-)

Parsival Wed 11 Feb 2009 04:15 AM #15

Thanks for all your help, friends.
S