Language: 
To browser these website, it's necessary to store cookies on your computer.
The cookies contain no personal information, they are required for program control.
  the storage of cookies while browsing this website, on Login and Register.

Author Topic:  Overhaul of /ban code  (Read 12491 times)

0 Members and 0 Guests are viewing this topic.

Torchwood

« Reply #15 on: 02, March 2012, 21:42:18 »
Sorry, also ...

I've updated bug-tracker #26, which requests account banning.  I put a 'close request' on this one, but I don't know how that works?  Who closes?

Not sure about #25; the only thing I can spot there still open is the ability to set the ban time as "15d" or "10h".  At the moment, you can only specify a number, which is translated as a number of seconds.  Do we want the ability to type "15d" to ban for 15 days?

clobber

« Reply #16 on: 02, March 2012, 22:50:35 »
Sounds nice and useful but it isn't vital.
Posted by Clobber

Collector Of Burnt out torches, 0 and Counting.

,-.  ___ ,-.
 \/ .   .  \ / 
(___O___)
 /  \      /   )
 ( ||       || )
  000     000
Woof, Woof!

Quote from: Longir
I use caution, fear is a distraction

Torchwood

« Reply #17 on: 03, March 2012, 08:04:47 »
:)

Agreed - not vital.  But, remember, my server coding skills are still pretty minimal.  I really don't understand many, many areas of the code yet, and my C skills are fairly basic.  So stuff like this forms a relatively safe area for me to work in.  I'm unlikely to mess up any player files or to break anything, and I'm slowly getting to grips with more aspects of the code.

Of course the downside is I'm not likely to be able to address any of, say Immage's, feedback at the moment.  But I hope that will come in time.  One day I want to be able to add something major to the code ...

Anyway, r6844 adds the ability to ban for 1d, 1h, 1m or 1s.  It is actually a pretty simple piece of code (but useful - I learnt something new).

:)

clobber

« Reply #18 on: 04, March 2012, 01:45:25 »
*cough* clans *cough*
Posted by Clobber

Collector Of Burnt out torches, 0 and Counting.

,-.  ___ ,-.
 \/ .   .  \ / 
(___O___)
 /  \      /   )
 ( ||       || )
  000     000
Woof, Woof!

Quote from: Longir
I use caution, fear is a distraction

Torchwood

« Reply #19 on: 04, March 2012, 07:32:27 »

Hmmm ... maybe?

Make that coughing noise again in about a month to remind me.

clobber

« Reply #20 on: 04, March 2012, 14:43:46 »
Great :D
Posted by Clobber

Collector Of Burnt out torches, 0 and Counting.

,-.  ___ ,-.
 \/ .   .  \ / 
(___O___)
 /  \      /   )
 ( ||       || )
  000     000
Woof, Woof!

Quote from: Longir
I use caution, fear is a distraction

Torchwood

« Reply #21 on: 04, March 2012, 20:26:26 »
Wow - that was hard work!!!

I decided to try and write find_players_on_ip(str), for use within /ban.  After several hours scratching my head (and a few 'reverts' - 'lets start again' moments), I've figured out how to create a temporary linked list.

r6847 adds the new code; in the end it was relatively simple.  However, I would really appreciated it if someone more experienced than me could cast their eyes over the code and check I haven't created any memory leaks anywhere?

However, in testing this code I found a bug.  It took me several hours to realise the bug is in the existing doeric_server() code, and actually nothing to do with my new code.

This function contains this code:
Code: [Select]
/* Go through the players.  Let the loop set the next pl value,
 * since we may remove some
 */
for (pl = first_player; pl != NULL;)
{
    if (pl->socket.status == Ns_Dead)
    {
        player *npl = pl->next;

        remove_ns_dead_player(pl);
        pl = npl;
    }

Now, when we /ban an IP address, my code searches for all characters on that IP address and /kicks them (similarly when banning an account).

The problem comes in that if a person has logged into the server twice using 2 different characters, it is possible these 2 characters are next to each other in the server's list of players.  In this case, when both characters are kicked, the reference to pl->next won't be valid in the code above!  This causes a sigfault in the server.

In the past, you would /kick one player, and then /kick the second player in a separate command, and doeric_server has a chance to tidy stuff up in between the 2 command calls.  Now we have both /kicks inside the same command call, and doeric_server then gets it wrong!

For now, the /kick bit of my new code for IPs and Accounts is commented out.

Any ideas how we fix this?
« Last Edit: 04, March 2012, 20:30:49 by Torchwood »

smacky

« Reply #22 on: 04, March 2012, 23:18:56 »
Add Ns_Kicked then instead of actually kicking the player/ip just change the socket.status to Ns_Kicked. Then the above if block tests for Ns_Dead || Ns_Kicked, IOW kick/ban/whatever just marks the perp, do_eric_server() does the actual kicking. (IDK if you even need to add Ns_Kicked, perhaps just setting NsDead works, not sure which is best)

_people_

« Reply #23 on: 05, March 2012, 03:03:15 »
Another issue I've just found is that in many recent revisions, I cannot create a new character. I get a message saying something about error code 2, call a GM.
-- _people_ :)

smacky

« Reply #24 on: 05, March 2012, 04:10:52 »
Is this on a public server (which one?) or local?

Error 2 is ADDME_MSG_CORRUPT which is an unhelpfully generic error but seems most often to indicate the file could not be saved for some reason. Lack of disk space?

EDIT: Yes, I get this on dev 100% of the time. Main is fine though.
« Last Edit: 05, March 2012, 04:19:43 by smacky »

_people_

« Reply #25 on: 05, March 2012, 04:15:39 »
I have plenty of disk space. I grabbed another revision a ways back and it worked fine. I checked the account file and it contained nothing but my password.

It was on local only, but it might be on Dev too if it was rebooted since whenever it broke. EDIT: Yep, broken on Dev too.
« Last Edit: 05, March 2012, 04:18:07 by _people_ »
-- _people_ :)

smacky

« Reply #26 on: 05, March 2012, 04:50:08 »
Here's my guess: http://daimonin.svn.sourceforge.net/viewvc/daimonin?view=revision&sortdir=down&revision=6837

These new functions in account.c are called from login.c:player_save(), causing iit to fail iif the player is not in an account. Which for just created chars is the case. So 100% failure. In commands.c:cs_cmd_newchar() is:

Code: [Select]
            if(!player_save(pl->ob)) /* if we can't save we don't add this char to the account */
                ret = ADDME_MSG_CORRUPT;

Torchwood

« Reply #27 on: 05, March 2012, 07:59:03 »
Ooops - sorry guys.

Yes - you are right smacky.  I replicated the fault on my local server.  I temporarily disabled the account_save inside player_save, and problem goes away.  I've just committed this and rebooted DEV, so when that comes back up we should be fine.

I'll investigate the proper fix this evening; I guess should be simple enough.

Torchwood

« Reply #28 on: 05, March 2012, 17:35:39 »

Properly fixed now with r6856, although I haven't rebooted DEV yet.

Torchwood

« Reply #29 on: 05, March 2012, 19:56:54 »
Add Ns_Kicked then instead of actually kicking the player/ip just change the socket.status to Ns_Kicked. Then the above if block tests for Ns_Dead || Ns_Kicked, IOW kick/ban/whatever just marks the perp, do_eric_server() does the actual kicking. (IDK if you even need to add Ns_Kicked, perhaps just setting NsDead works, not sure which is best)

Just been looking at this, and I know why we get the crash.  In pseudocode, we have this:

Code: [Select]
command_ban()
{
kick_player(pl1)
{
remove_ob(pl1)  // marks pl1 as eligible for removal
}
kick_player(pl2)
{
remove_ob(pl2)  // marks pl2 as eligible for removal
}
}

doeric_server()
{
remove_ns_dead_player(pl1)
remove_ns_dead_player(pl2)
}


remove_ns_dead_player(pl)
{
...
leave_map(pl)
{
remove_ob(pl)
}
...
free_player(pl)
{
object_gc()  // removes ALL objects marked as eligible for removal
}
}

So, the ban command ends up calling remove_ob() on each player, which means the call to object_gc() in the first call to remove_ns_dead_player() ends up wiping out both players at the same time.  The second call to remove_ns_dead_player() then has an invalid pointer, and we crash.

Solution is simple - get rid of the call to remove_ob() inside kick_player().  We can safely do this because remove_ns_dead_player() calls remove_ob() anyway via leave_map() prior to free_player().

Hope that all makes sense?  I've tested it and all seems to work, but I'm not going to commit just yet ... would like some opinions first?

Tags:
 

Related Topics

  Subject / Started by Replies Last post
27 Replies
9367 Views
Last post 06, October 2005, 21:32:54
by Antoneja
0 Replies
857 Views
Last post 31, January 2007, 19:31:06
by smacky
14 Replies
2901 Views
Last post 05, July 2008, 03:49:39
by zaurbock
9 Replies
6996 Views
Last post 05, October 2011, 00:59:33
by _people_
36 Replies
35766 Views
Last post 19, July 2012, 20:37:29
by Torchwood