Hi Petr,

The modification I propose in http://moodle.org/mod/forum/discuss.php?d=80104#p551492 fixes the issue for the 0.0.0.0/0 subnet.

Saludos.
Iñaki.

thanks

Fixed in CVS for both 1.8.x and 1.9.x (HEAD is not affected with this issue).

Alastair, thanks a lot Alastair for the bug report!

Saludos.
Iñaki.

address_in_subnet has extensive unit tests. Please run them and make sure they all pass before changing this function.

(They don't but I think the bug is in the test not the real code. Still it needs to be thought about and fixed.)

Thanks.

The bug is clearly in the unit tests . If you have a look at RFC 4632, section 3.1.(Basic Concept and Prefix Notation), the CIDR syntax specifies that there has to be a decimal value between 0 and 32 (both included) after the '/' separator, and that '/0' is a netmask that means no significant bits at all (i.e., it matches any subnet or address). So at lest the following unit tests are wrong:

$this->assertTrue(address_in_subnet('123.121.234.0', '123.121.234.0/')); // / is like /32.
$this->assertFalse(address_in_subnet('123.121.234.1', '123.121.234.0/'));

as there should be a decimal value after the subnet mask separator. Although I'd prefer to signal a syntax error (as in the case of the last unit test with an invalid netmask of '/148'), we should at least return False.

$this->assertFalse(address_in_subnet('232.232.232.232', '123.121.234.0/0'));

As this check should return True ('/0' is the wildcard mask for 'any address', as mentionned above).

Saludos.
Iñaki.

By the way, if we agree that this test is wrong (and I do):

$this->assertTrue(address_in_subnet('123.121.234.0', '123.121.234.0/')); // / is like /32.

we should not only fix the unit test, but also the code in address_in_subnet().

In the second test:

$this->assertFalse(address_in_subnet('123.121.234.1', '123.121.234.0/'));

if we are just going to return false for invalid syntax subnet masks (like we currently do for netmasks outside of the 0-32 range), then the unit test is right and shouldn't be fixed.

Saludos.
Iñaki.

The one place where this is explained in documentation that I know is http://cvs.moodle.org/moodle/lang/en_utf8/help/quiz/requiresubnet.html?revision=1.3&view=markup, so we should not do anything that breaks backwards compatibility with that.

Fortunately, we aren't going to do that.

I guess false on invalid input, is a sensible policy. Please can you update the tests and the code.

Code and tests updated in 1.8.x and 1.9.x, using the same code and functions that are in HEAD.

Closing now so Tim can do the QA

Saludos.
Iñaki.

Sorry, I am still not happy. You seem to have invented a function is_number that does exactly the same things is the PHP built-in function is_numeric.

(Also, when you fix a bug, you should resolve it, not close it. The person who does QA closes it.)

Hi Tim,

regarding the resolve vs close, you are right (I cliked on the wrong link, my fault).

As to the is_numeric PHP built-in function, we can't/shouldn't use it, because it also accepts floats as valid values, which is wrong in this case (we should only accept integer values). On the other hand I haven't invented the is_number() function, I've copied it verbatim from HEAD and copied the address_in_subnet check for IPv4 addresses almost verbatim from HEAD.

So if Petr (as the author of the code in HEAD according to cvs annotate) and you could make up your mind on how to do it, I'd gladly fix the code in The Right Way(tm)

Saludos.
Iñaki.

Is 'is_int' any use?
http://uk.php.net/is_int

Alastair

Hi Alastair,

I'm afraid it isn't, as the mask is actually a string (even if it represents an integer value). So is_int() would always return false.

Saludos.
Iñaki.

Doh! sorry for messing you around Iñaki. You are right and I should have been playing more attention.

Don't worry Tim. Better safe than sorry

Saludos.
Iñaki.