Overhaul of /ban code

[Torchwood]

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]

Sounds nice and useful but it isn't vital.

[Torchwood]

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]

*cough* clans *cough*

[Torchwood]

Hmmm ... maybe?

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

[clobber]

Great

[Torchwood]

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?

[smacky]

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_]

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.

[smacky]

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.

[_people_]

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.

[smacky]

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]

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]

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

[Torchwood]

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?