Division by zero FPD

[Destroy666x]

All found issues originate from the maxwarningpoints setting being equal to zero and emit a PHP warning Division by zero.

https://github.com/mybb/mybb/blob/feature/admin/modules/user/users.php#L1056
https://github.com/mybb/mybb/blob/feature/admin/modules/user/users.php#L3570

https://github.com/mybb/mybb/blob/feature/inc/datahandlers/warnings.php#L373
https://github.com/mybb/mybb/blob/feature/inc/datahandlers/warnings.php#L374
https://github.com/mybb/mybb/blob/feature/inc/datahandlers/warnings.php#L561
https://github.com/mybb/mybb/blob/feature/inc/datahandlers/warnings.php#L562

https://github.com/mybb/mybb/blob/feature/inc/functions_post.php#L377

https://github.com/mybb/mybb/blob/feature/member.php#L2430

https://github.com/mybb/mybb/blob/feature/usercp.php#L3694

https://github.com/mybb/mybb/blob/feature/warnings.php#L344
https://github.com/mybb/mybb/blob/feature/warnings.php#L411
https://github.com/mybb/mybb/blob/feature/warnings.php#L741

repsperpage setting-related:
https://github.com/mybb/mybb/blob/feature/reputation.php#L835
https://github.com/mybb/mybb/blob/feature/reputation.php#L854 (zero as an argument)

membersperpage setting-related:
https://github.com/mybb/mybb/blob/feature/managegroup.php#L355 (zero as an argument)

Original thread: Division by zero FPD

[dvz]

Added the second part;
in case of repsperpage and membersperpage the value would be defaulted like in the other places; I guess we should do the same with maxwarningpoints (default is 10)?

[Destroy666x]

I guess we should do the same with maxwarningpoints (default is 10)

Probably, if we want to be consistent at least. I'd also add a minimum note to that setting's description because some admins may think that 0 disables the maximum.

As for the commits you made - I think the check should be rather something like:

$perpage = (int)$mybb->settings['repsperpage'];
if($perpage < 1)

to be completely safe. Because right now someone can change it to -1 or anything stupid and this may lead to unexpected behavior (or even a MySQL error because of invalid LIMIT).

[dvz]

Integers can be negative, so that would have to go by abs((int)$mybb->settings['repsperpage']).

Doing the thing with maxwarningpoints will eliminate the math issue but is likely to cause unpredictable behavior (the value of 0 won't produce errors but the application logic might fail).

[Destroy666x]

Integers can be negative

That wouldn't make any sense in any of the settings mentioned in this issue though - per page and warning points are supposed to be always positive.

so that would have to go by abs((int)$mybb->settings['repsperpage'])

I don't see the need of abs() when there's an if check that reverts the setting to default if a non-positive value is entered for some reason. We could make the number positive instead, yes, but that doesn't really matter - both default and absolute will use other number than the one specified in ACP anyways. And we already use the default in these cases in some places, for example: https://github.com/mybb/mybb/blob/feature/showthread.php#L364

[dvz]

That wouldn't make any sense in any of the settings mentioned in this issue though - per page and warning points are supposed to be always positive.

True, but it should be corrected anyway because it emits a PHP error, leading to FPD (you never know when it might get in handy for someone).

I don't see the need of abs() when there's an if check that reverts the setting to default if a non-positive value is entered for some reason.

In this case there is no such check ((bool)(int)'-1', an equivalent of that IF, returns true). Many examples related to LIMIT statements: http://community.mybb.com/thread-169555-post-1154569.html#pid1154569).

[Destroy666x]

True, but it should be corrected anyway because it emits a PHP error, leading to FPD
In this case there is no such check ((bool)(int)'-1', an equivalent of that IF, returns true).

I'm still not sure what you mean, -1 (or anything less than 1) in the setting with the code I suggested above would be changed to the default 20, which would always prevent FPD - or am I missing something?

[dvz]

Right, ($perpage < 1) would work (I must have been looking at a different fragment - (!$perpage), there are lots of these - changing to < 1 would do the trick).