On Fri, 2017-11-17 at 12:42 -0800, Jennifer wrote:
> Hello,
>
> I have a script that compares a visitors IP address against a
> file of known bad addresses but I'm getting this notice:
>
> Notice: Undefined offset: 3 (line 26 below)
>
> Can anyone see what I've done wrong? Perhaps this sub was not
> written correctly to begin with?
>
> Thank you,
> Jenni
>
> -----------------------------------------------------------
>
> function validate_ip($ip_array, $cm) {
> $msg = '';
> if (isset($cm['bad_ip_message'])) {
> $msg = $cm['bad_ip_message'];
> }
> if (@strpos( $cm['remote_ip'], ':' ) !== false) { return false;
> } // Skip IPv6 addresses (?).
>
> if (!is_array($ip_array)) { return false; }
> foreach ($ip_array as $banned_ip) {
> $banned_ip = trim($banned_ip);
> if ($banned_ip[0] === '#' || $banned_ip === '') {
> continue; } // Skip comments and blank lines.
>
> if ( @strpos( $banned_ip, '/' ) === false ) {
> if (ip2long($banned_ip) ===
> ip2long($cm['remote_ip'])) {
> ban_message($msg);
> }
> if (substr($banned_ip, -1) == '0') {
> $banned_ip .= '/24'; // Create a CIDR
> if IP ends in '0'.
> }
> }
> if ( @strpos( $banned_ip, '/' ) !== false ) {
> // Get the base and the bits from the CIDR.
> list($base, $bits) = explode('/', $banned_ip);
>
> // Now split it up into it's classes.
> list($a, $b, $c, $d) = explode('.', $base); //
> Notice: Undefined offset: 3
>
> // Now do some bit shifting/switching to
> convert to ints.
> $i = ($a << 24) + ($b << 16) + ($c << 8) +
> $d;
> $mask = $bits == 0 ? 0: (~0 << (32 - $bits));
>
> // Here's our lowest int.
> $low = $i & $mask;
>
> // Here's our highest int.
> $high = $i | (~$mask & 0xFFFFFFFF);
>
> // Now split up the IP we're checking against
> into classes.
> list($a, $b, $c, $d) = explode('.',
> $cm['remote_ip']);
>
> // Now convert the IP we're checking against to
> an int.
> $check = ($a << 24) + ($b << 16) + ($c << 8) +
> $d;
>
> // If the IP is within the range, including
> highest/lowest values,
> // then it's within the CIDR range.
> if ($check >= $low && $check <= $high) {
> ban_message($msg);
> }
> }
> }
> return;
> }
Please never, never use the `@` to suppress error messages. It hides
errors and warnings, and there's frankly not one good reason to use
them.
As to your problem, you haven't said what you're feeding into this.
Have you tried any sort of debugging yet to see why `$base` doesn't
have 3 '.' characters?
This probably could be written a bit better. Exploding strings into
parts (where you really only want one substring) is expensive in terms
of CPU cycles, compared to a straight up substring. You could even
consider using regular expressions, as they would allow you to perform
the check for a specific character and the substring extraction in one
line.
Also, have you considered converting the whole IP address into a single
large integer? It looks like you're splitting up the address,
converting each part to a shifted value, and then combining them again
(`$i = ($a << 24) + ($b << 16) + ($c << 8) + $d;`). `ip2long()` will
do this (you're already specifically ignoring IPv6 addresses, so this
will suit your purposes). It will be far faster than what you're doing,
and make the code a lot simpler and easier to read/manage.
--
Thanks,
Ash
http://www.ashleysheridan.co.uk
