Problems detecting multiple clients

[samasama]

Hi...
I'm trying to detect multiple clients connected to a SOCK_STREAM
server. I'm doing this via a hash. The actual detecting part is going
fine. The problem I'm running into however takes place when a second
client connects from the same ip address (multiple clients)... When a
second connection is made from the same host, it errors out and
disconnects that second connection as it should, but when the first
(original) connection disconnects and tries to re-connect it doesn't
work, and gets treated as a second connection. This is because the
first (original) connection key in the hash does not get deleted. I've
been banging my head into the desk for a few weeks trying to figure out
how to get that key deleted, and properly.

Any help, advice, pointers, brutal criticism is very much welcome and
appreciated.



#!/usr/bin/perl

use warnings;
use diagnostics;
use strict;
use IO::Socket;
use POSIX qwsys_wait_h);

our %ip_conns; # hash of connected clients

our $time_to_die = 0;

# Globals for the socket
our $SERVER;
our $CLIENT;

$SIG{CHLD} = \&handle_sigchld;

$SIG{INT} = $SIG{TERM} = \&handle_default;

our $banner =
"\%rwhois V-1.5:000082:00 rwhois.foo.bar (Rwhois Server version)";

our $foo; # Temp for messing with chk_multi_conn

my $pid = fork;
exit if $pid;
die "Couldn't fork: $!\n" unless defined($pid);

POSIX::setsid() || die "Can't start a new session: $!";

$SERVER = IO::Socket::INET->new(
Proto => 'tcp',
LocalAddr => '0.0.0.0',
LocalPort => '4321',
Listen => SOMAXCONN,
Reuse => 1
);
die "WSRwhois : Can't setup server : $!\n " unless $SERVER;
print "[ Server $0 accepting clients ]\n";


until ($time_to_die) {

while ( $CLIENT = $SERVER->accept() ) {
$foo = $CLIENT;
$ip_conns{$CLIENT} = $CLIENT->peerhost; # key it
my $conn_ret = chk_multi_conn(); # Check it
if ($conn_ret == 1 ) {
delete $ip_conns{$CLIENT};
shutdown($CLIENT, 2); }

my $kidpid = fork;
die "Fork: $!\n" unless defined($kidpid);

if ( $kidpid == 0 ) {
shutdown( $SERVER, 1 );
$CLIENT->autoflush;
print $CLIENT "$banner\015\012";

while ( sysread( $CLIENT, $_, 1024 ) ) {
next unless /\S/;

if (/-quit/) {
print $CLIENT "\%ok\015\012";
shutdown( $CLIENT, 2 );
}

}
delete $ip_conns{$CLIENT};
exit(0);

}

else { # Let the parent do some stuff

}

}

}

sub chk_multi_conn {
my %count = ();
my $times;
foreach my $keys ( keys %ip_conns ) {
my $value = $ip_conns{$keys};
print "#DEBUG Inside chk_multi_conn()\n"; # DEBUG
print "#DEBUG \%ip_conns = $keys => $value\n"; # DEBUG
print "#DEBUG\n"; # DEBUG
print "#DEBUG \$CLIENT = $CLIENT\n"; # DEBUG
$times = $count{$value}++;
}
if ( $times >= 1 ) {
print $CLIENT
"\%error Only 1 connection allowed per host.\n";
return (1);
}
else {
return (0); }

}

sub handle_sigchld {
delete $ip_conns{$foo}; # Just to be sure
my $pid = wait; # Wait for children to die
$SIG{'CHLD'} = \&handle_sigchld;
}

sub handle_default {
$time_to_die = 1; # Kill daemon
}

 

[xhoster]

Hi...
I'm trying to detect multiple clients connected to a SOCK_STREAM
server. I'm doing this via a hash. The actual detecting part is going
fine. The problem I'm running into however takes place when a second
client connects from the same ip address (multiple clients)... When a
second connection is made from the same host, it errors out and
disconnects that second connection as it should, but when the first
(original) connection disconnects and tries to re-connect it doesn't
work, and gets treated as a second connection. This is because the
first (original) connection key in the hash does not get deleted.

You have a forking server. You are deleting the entry from the child's
hash, which has no affect on the parent's hash.

Any help, advice, pointers, brutal criticism is very much welcome and
appreciated.

#!/usr/bin/perl

use warnings;
use diagnostics;
use strict;
use IO::Socket;
use POSIX qwsys_wait_h);

our %ip_conns; # hash of connected clients

our $time_to_die = 0;

# Globals for the socket
our $SERVER;
our $CLIENT;

Why do you have all the "our" rather than using "my"? $SERVER and $CLIENT
probably shouldn't be declared so early.

$SIG{CHLD} = \&handle_sigchld;

$SIG{INT} = $SIG{TERM} = \&handle_default;

our $banner =
"\%rwhois V-1.5:000082:00 rwhois.foo.bar (Rwhois Server version)";

our $foo; # Temp for messing with chk_multi_conn

Why do you think this is necessary?

....

sub chk_multi_conn {
my %count = ();
my $times;
foreach my $keys ( keys %ip_conns ) {
my $value = $ip_conns{$keys};
print "#DEBUG Inside chk_multi_conn()\n"; # DEBUG
print "#DEBUG \%ip_conns = $keys => $value\n"; # DEBUG
print "#DEBUG\n"; # DEBUG
print "#DEBUG \$CLIENT = $CLIENT\n"; # DEBUG
$times = $count{$value}++;
}

I don't see how this is supposed to do anything useful at all. The final
value of $times depends on what order the hash keys are stored it, which
is indeterminate. Besides, Wouldn't it make a lot more sense to reverse
the %ip_conns, so you can just look up into it using the rather than
looping over it?

if ( $times >= 1 ) {
print $CLIENT
"\%error Only 1 connection allowed per host.\n";
return (1);
}
else {
return (0); }

}

sub handle_sigchld {
delete $ip_conns{$foo}; # Just to be sure

At this point, $foo just contains the last connection accepted. There is
no particular reason to think the connection just ended is the same as the
last one accepted, so you are deleting the wrong thing

my $pid = wait; # Wait for children to die

What you need to do is keep a hash mapping child pid to child connection
IP, so that you can lookup what connection just ended according to which
child just exited.

Also, you should use waitpid in a loop, rather than wait. If two children
exit in close proximity, your handle might only get called one time, so
only one of them would be harvested.

Xho

 

[samasama]

(e-mail address removed) wrote:

Keep in my mind I'm a big friggin noob, though from my code I'm sure
that's obvious
This is also my first program.

You have a forking server. You are deleting the entry from the child's
hash, which has no affect on the parent's hash.

I was deleting here...
while ( $CLIENT = $SERVER->accept() ) {
$foo = $CLIENT;
$ip_conns{$CLIENT} = $CLIENT->peerhost;
my $conn_ret = chk_multi_conn();
if ($conn_ret == 1 ) {
delete $ip_conns{$CLIENT};
shutdown($CLIENT, 2); }

Which would be in the parent... The other deletes was shotgun
troubleshooting...

I thought this was the proper way to delcare globals?
Also, this is a broken down version of 900 line program. I needed to
delcare them early for use in sub throughout the parent and child.

Why do you have all the "our" rather than using "my"? $SERVER and $CLIENT
probably shouldn't be declared so early.

I thought this was the proper way to delcare globals?
Also, this is a broken down version of 900 line program. I needed to
delcare them early for use in sub throughout the parent and child.

Why do you think this is necessary?

Once again, voodoo programming and shotgun troubleshooting
I got to the point where I was trying anything.
I thought perhaps handle_sigchld would/was get confused if I used
$CLIENT

...

I don't see how this is supposed to do anything useful at all. The final
value of $times depends on what order the hash keys are stored it, which
is indeterminate. Besides, Wouldn't it make a lot more sense to reverse
the %ip_conns, so you can just look up into it using the rather than
looping over it?

I don't understand : )
I pulled that out of the cookbook, for checking dups in a hash.

At this point, $foo just contains the last connection accepted. There is
no particular reason to think the connection just ended is the same as the
last one accepted, so you are deleting the wrong thing
*nod*


What you need to do is keep a hash mapping child pid to child connection
IP, so that you can lookup what connection just ended according to which
child just exited.

hehe, huh?

Also, you should use waitpid in a loop, rather than wait. If two children
exit in close proximity, your handle might only get called one time, so
only one of them would be harvested.

Noted, and thanks for pointing that out.

Thanks period : )

 

[xhoster]

(e-mail address removed) wrote:

Keep in my mind I'm a big friggin noob, though from my code I'm sure
that's obvious
This is also my first program.


I was deleting here...
while ( $CLIENT = $SERVER->accept() ) {
$foo = $CLIENT;
$ip_conns{$CLIENT} = $CLIENT->peerhost;
my $conn_ret = chk_multi_conn();
if ($conn_ret == 1 ) {
delete $ip_conns{$CLIENT};
shutdown($CLIENT, 2); }

Which would be in the parent... The other deletes was shotgun
troubleshooting...

That only deletes connections which are denied due to chk_multi_conn. What
about the connections that are acceptedable and then at some point down the
road get closed naturally?

I thought this was the proper way to delcare globals?
Also, this is a broken down version of 900 line program. I needed to
delcare them early for use in sub throughout the parent and child.

You might want to pass them into the subs, instead of using them globally.
Yes, it can be more annoying to write initially (and no, I don't always
follow my own advice), but it is generally worth it in the long run.

I don't understand : )
I pulled that out of the cookbook, for checking dups in a hash.

I don't know cookbook it is from, or what it looks like originally, but
I'm pretty sure it didn't look like that. You must have screwed something
up in the conversion. One thing to note is that $CLIENT doesn't appear
productively anywhere in that code. How can it tell if CLIENT shows up
more than once when it doesn't even care about what CLIENT is?

What you end up with is that %count holds the count of the number of times
each IP address is connected, and $times holds that count for one
(unpredictably) arbitrary IP address.

hehe, huh?

after you fork, you do something like
$pid_list{$pid}=$CLIENT->peerhost;

Then when "wait" gives you a pid, you look it up in the %pid_list hash to
see what IP needs to be deleted from %ip_conns.

Xho

 

[samasama]

That only deletes connections which are denied due to chk_multi_conn. What
about the connections that are acceptedable and then at some point down the
road get closed naturally?

*nod*, that's why I was messing with other deletes, which with what you
already said didn't make any sense at all. Noted.

You might want to pass them into the subs, instead of using them globally.
Yes, it can be more annoying to write initially (and no, I don't always
follow my own advice), but it is generally worth it in the long run.

I'll try that, I'm not worried about annoyances, just writing better
code.

I don't know cookbook it is from, or what it looks like originally, but
I'm pretty sure it didn't look like that. You must have screwed something
up in the conversion. One thing to note is that $CLIENT doesn't appear
productively anywhere in that code. How can it tell if CLIENT shows up
more than once when it doesn't even care about what CLIENT is?

What you end up with is that %count holds the count of the number of times
each IP address is connected, and $times holds that count for one
(unpredictably) arbitrary IP address.

First edition, and yes I mangled it a bit...

From the cookbook:

%count = ();
foreach $element (@ARRAY) {
$count{$element}++;
}

after you fork, you do something like
$pid_list{$pid}=$CLIENT->peerhost;

Then when "wait" gives you a pid, you look it up in the %pid_list hash to
see what IP needs to be deleted from %ip_conns.

Ahhhhhh! Ok that makes sense!

Thanks so much for your help.
I'm sure I'll be back following up in a day or two with progress
hopefully : )

 

[samasama]

*nod*, that's why I was messing with other deletes, which with what you
already said didn't make any sense at all. Noted.



I'll try that, I'm not worried about annoyances, just writing better
code.

Just wanted to go over this...

"You have an aggregate data structure, such as an array or a hash. You
want to know how often each element in the array (or key or value in
the hash) occurs. For instance, if your array contains web server
transactions, you might want to find the most commonly requested file.
If your hash maps usernames to number of logins, you want to find the
most common number of logins."

and below is the solution...

First edition, and yes I mangled it a bit...

%count = ();
foreach $element (@ARRAY) {
$count{$element}++;
}

in my sub i'm doing foreach my $keys ( keys %ip_conns ) { ...

You're saying this is wrong? Or can have a bad side effect?


I did this, and it's working : )

How I did it:

I already had a hash setup to record $kidpid, and just added the ip
address on to it ($children{$kidpid} = $CLIENT->peerhost

So, I do this within my handle_sigchld sub...

my $pid = waitpid(-1, WNOHANG); # Wait for children to die
foreach my $keys (keys %children) {
my $value = $children{$keys};
if ($pid == $keys) {
my %temp = reverse %ip_conns;
delete $temp{$value};
%ip_conns = reverse %temp;
}
}

It's working flawlessly (I believe)...

Thanks so much, and any more feedback/corrections/whatever is vastly
appreciated.

 

[xhoster]

Just wanted to go over this...

"You have an aggregate data structure, such as an array or a hash. You
want to know how often each element in the array (or key or value in
the hash) occurs. For instance, if your array contains web server
transactions, you might want to find the most commonly requested file.
If your hash maps usernames to number of logins, you want to find the
most common number of logins."

and below is the solution...


in my sub i'm doing foreach my $keys ( keys %ip_conns ) { ...

You're saying this is wrong? Or can have a bad side effect?

Nope, that part is right. Later on, you had

$times = $count{$value}++;

Once the loop ends, $times is equal to the count of the last $value seen
(actually, one minus that count, as you use postincrement rather than
preincrement.) But you don't know what the last $value seen is, because
the order in which they are seen is arbitrary.

But since you don't seem to use the keys but only the values,
you could make the IP address, rather than the socket, be the key to the
%ip_conns hash. Then you don't need to loop at all, you just look up into
the hash. (This would require you to rearrange the code. Currently you
add to %ip_conns, then test for duplicates. You would have to first test
for duplicates, then add to %ip_conns only if it is not already there.)

I did this, and it's working : )

How I did it:

I already had a hash setup to record $kidpid, and just added the ip
address on to it ($children{$kidpid} = $CLIENT->peerhost

So, I do this within my handle_sigchld sub...

my $pid = waitpid(-1, WNOHANG); # Wait for children to die
foreach my $keys (keys %children) {
my $value = $children{$keys};
if ($pid == $keys) {
my %temp = reverse %ip_conns;
delete $temp{$value};
%ip_conns = reverse %temp;
}
}

It would probably make sense to have %ip_conns be permanently reversed,
i.e. have the IP be the key and socket be the value (or the value could
just be "1" or undef, since the socket part which is stored in that hash
is never used anyway (and indeed probably can't be used--hash keys
are stringifed and thus lose their magic.))

Xho

 

[samasama]

Nope, that part is right. Later on, you had

$times = $count{$value}++;

Once the loop ends, $times is equal to the count of the last $value seen
(actually, one minus that count, as you use postincrement rather than
preincrement.) But you don't know what the last $value seen is, because
the order in which they are seen is arbitrary.

But since you don't seem to use the keys but only the values,
you could make the IP address, rather than the socket, be the key to the
%ip_conns hash. Then you don't need to loop at all, you just look up into
the hash. (This would require you to rearrange the code. Currently you
add to %ip_conns, then test for duplicates. You would have to first test
for duplicates, then add to %ip_conns only if it is not already there.)

ahhhh, I'll give it a shot!

It would probably make sense to have %ip_conns be permanently reversed,
i.e. have the IP be the key and socket be the value (or the value could
just be "1" or undef, since the socket part which is stored in that hash
is never used anyway (and indeed probably can't be used--hash keys
are stringifed and thus lose their magic.))

Yes, this does make sense.

Thank you so much : )

 

[samasama]

It would probably make sense to have %ip_conns be permanently reversed,
i.e. have the IP be the key and socket be the value (or the value could
just be "1" or undef, since the socket part which is stored in that hash
is never used anyway (and indeed probably can't be used--hash keys
are stringifed and thus lose their magic.))

I had a "Oh yeah..." moment... One of the main reasons I'm keying by
$CLIENT is because $CLIENT will be unique every connection. If I were
to key by ip address, this may not be true, two connections from
127.0.0.1 will not get two seperate keys... AFAIK. I need it to be
unique every time, be able to create multiple keys for the same ip
address. So with that, keying by $CLIENT does the job I want.



Though, now I'm trying to figure out out how to remove a single
instance...

my $pid = waitpid(-1, WNOHANG); # Wait for children to die

foreach my $keys (keys %children) {
my $value = $children{$keys};
if ($pid == $keys) {
my %temp = reverse %ip_conns;
delete $temp{$value};
%ip_conns = reverse %temp;
}
}

This works perfectly for right now, by down the road I might want to
allow two connections from the same host at once. In this case I
believe I would need to delete only one instance of $value.
I just started thinking about that so... I'm sure the solution is
quite easy.
Perhaps I could key %ip_conns by $kidpid, hmmm, have to think about
that.

Anyway, just ranting : )

 

[xhoster]

I had a "Oh yeah..." moment... One of the main reasons I'm keying by
$CLIENT is because $CLIENT will be unique every connection. If I were
to key by ip address, this may not be true, two connections from
127.0.0.1 will not get two seperate keys...

I had thought that duplicate connections from the same IP were disallowed,
so that that wouldn't be an issue. Still, if you are allowing multiple
connections per IP, you could always make a hash mapping IP to count.

AFAIK. I need it to be
unique every time, be able to create multiple keys for the same ip
address. So with that, keying by $CLIENT does the job I want.

Though, now I'm trying to figure out out how to remove a single
instance...

my $pid = waitpid(-1, WNOHANG); # Wait for children to die

foreach my $keys (keys %children) {
my $value = $children{$keys};
if ($pid == $keys) {

There is no reason to loop over all keys if you only do anything with
one of those keys.

if (exists $children{$pid}) {
my $value = $children{$pid};
#...

my %temp = reverse %ip_conns;
delete $temp{$value};
%ip_conns = reverse %temp;

If you are going to keep %ip_conns like it is, then you should
store the socket (rather than the IP) in %children, then you could just do:
delete $ip_conns{$value};
Rather than all the reversing of entire hashes (which has added benefit of
not breaking if you allow multiple connects per IP, as you describe below).

}
}

This works perfectly for right now, by down the road I might want to
allow two connections from the same host at once. In this case I
believe I would need to delete only one instance of $value.
I just started thinking about that so... I'm sure the solution is
quite easy.
Perhaps I could key %ip_conns by $kidpid, hmmm, have to think about
that.

I still think you should change %ip_conns to be IP => count, rather than
socket=>IP, but if you keep it like it is you should store the socket,
rather than the IP, in %children.

Xho