Issue Details (XML | Word | Printable)

Key: MDL-15655
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Minor Minor
Assignee: Tim Hunt
Reporter: Daniel Pouliot
Votes: 0
Watchers: 1
Operations

Add/Edit UI Mockup to this issue
If you were logged in you would be able to see more operations.
Moodle

64 bit support for subnet mask of type 1 in quiz

Created: 15/Jul/08 09:44 PM   Updated: 01/Sep/08 12:15 PM
Return to search
Component/s: Quiz
Affects Version/s: 1.9.1
Fix Version/s: 1.7.6, 1.8.7, 1.9.3

Environment: 64 bit CPU architecture - Linux Intel

Participants: Daniel Pouliot and Tim Hunt
Security Level: None
Resolved date: 01/Sep/08
Affected Branches: MOODLE_19_STABLE
Fixed Branches: MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE


 Description  « Hide
When using quiz restriction by ip address, using range type 1: xxx.xxx.xxx.xxx/xx, on 64 bit CPU, result is unexpected. Add the '+' line :

+++ lib/moodlelib.php
@@ -7029,6 +7029,7 @@
         if (strpos($subnet, '/') !== false) { /// type 1
             list($ip, $mask) = explode('/', $subnet);
             $mask = 0xffffffff << (32 - $mask);
+ $mask = substr($mask,0,8); // Compatib. cpu 64 bit
             $found = ((ip2long($addr) & $mask) == (ip2long($ip) & $mask));
         } else if (strpos($subnet, '-') !== false) {/// type 3
             $subnetparts = explode('.', $subnet);


 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Tim Hunt made changes - 15/Jul/08 10:02 PM
Field Original Value New Value
Issue Type Improvement [ 4 ] Bug [ 1 ]
Tim Hunt added a comment - 20/Aug/08 06:05 PM
Thank you very much for reporting this issues, especially since you included a suggested fix. I'm sorry it took me so long to get around to looking at it.

Before applying your fix, I would like to understand it better. At the moment I am confused. All we are doing with $mask is &ing it with some 32 bit integers, so why should it matter if there are some extra bits set in $mask?

Would it be possible for you to try running the unit tests for this function on your 64-bit CPU? You just need to go to the URL .../admin/report/simpletest/index.php?&path=lib/simpletest/testmoodlelib.php on your moodle server. Do all those tests pass, both with and without your change?

One think about your fix that I don't like is the way it uses string manipulation on $mask. Wouldn't it be better to do $mask = $mask & 0xffffffff; instead of your line of code?


Daniel Pouliot added a comment - 29/Aug/08 10:45 PM
Full ip address restriction in quiz
ex.: quiz --> Require network address: 192.168.10.10

It will be interpreted as type 2 and will accept 192.168.10.10 and so 192.168.10.100 to 192.168.10.109

So we asked our teachers to use type 1 by adding a '/' at the end. The thing I don't remember is why we didn't ask them to add '/32'. Did it came from the doc? Note that the problem was treated on moodle 1.6.3 in 2007.

So, the difference on 64 bit appears when ending with '/'
ex.: quiz --> Require network address: 192.168.10.10/

This will work well on 32 bit cpu but will not work on 64 bit because of this instruction:
$mask = 0xffffffff << (32 - $mask);

On 32 bit, no shift will be done and $mask will get 0xFFFFFFFF
On 64 bit, a 32 bit shift will be apply giving the value of 0xFFFFFFFF00000000 to $mask

Conclusion:
'/' on 32 bit = '/32' and on 64 bit = '/0'.
'/32' on 32 and 64 bit = '/32'.

After many tests and research, I suggest to ignore this patch and add '/32' for full ip addresses. The doc need to be change or the code for type 2 when using full ip addresses need change. In this last case, may be using '*' for partial addresses would be a good solution.


Tim Hunt added a comment - 01/Sep/08 12:15 PM
Thank you for the clear analysis and explanation. I think I understand now.

I did not realise that / was valid syntax. I guess that the best thing is to explicitly check for this case, and treat it as /32.

Also, it was a bug if 192.168.10.10 matched 192.168.10.100. Type 2 should only apply to entire parts of the address, it should not be a general sub-string mechanism.

I have added unit tests for these cases, and then changed the code so that they all pass.

Hopefully that fixes the problem, however I don't have an easy way to test on a 64 bit architecture, so it would be good if you could. Thanks.


Tim Hunt made changes - 01/Sep/08 12:15 PM
Fix Version/s 1.8.7 [ 10291 ]
Fix Version/s 1.9.3 [ 10290 ]
Resolution Fixed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
Fix Version/s 1.7.6 [ 10292 ]
tjhunt committed 2 files to 'Moodle CVS' on branch 'MOODLE_17_STABLE' - 01/Sep/08 12:17 PM
MDL-15655 - address_in_subnet does not work on 64 bit architectures. Also, we found some other issues with this funtion while diagnosing the problem.
MODIFY lib/moodlelib.php   Rev. 1.774.2.41    (+9 -3 lines)
MODIFY lib/simpletest/testmoodlelib.php   Rev. 1.1.2.2    (+7 -1 lines)
tjhunt committed 2 files to 'Moodle CVS' - 01/Sep/08 12:17 PM
MDL-15655 - address_in_subnet does not work on 64 bit architectures. Also, we found some other issues with this funtion while diagnosing the problem.
MODIFY lib/moodlelib.php   Rev. 1.1101    (+9 -3 lines)
MODIFY lib/simpletest/testmoodlelib.php   Rev. 1.13    (+7 -1 lines)
tjhunt committed 2 files to 'Moodle CVS' on branch 'MOODLE_19_STABLE' - 01/Sep/08 12:17 PM
MDL-15655 - address_in_subnet does not work on 64 bit architectures. Also, we found some other issues with this funtion while diagnosing the problem.
MODIFY lib/moodlelib.php   Rev. 1.960.2.91    (+9 -3 lines)
MODIFY lib/simpletest/testmoodlelib.php   Rev. 1.9.2.3    (+7 -1 lines)
tjhunt committed 2 files to 'Moodle CVS' on branch 'MOODLE_18_STABLE' - 01/Sep/08 12:18 PM
MDL-15655 - address_in_subnet does not work on 64 bit architectures. Also, we found some other issues with this funtion while diagnosing the problem.
MODIFY lib/moodlelib.php   Rev. 1.837.2.91    (+9 -3 lines)
MODIFY lib/simpletest/testmoodlelib.php   Rev. 1.2.4.8    (+7 -1 lines)