Skip to content

Fatal error in my_gcd() on PHP 8: DivisionByZeroError: Modulo by zero #108

Description

@E1NSER

Plugin version 1.9.7, WordPress 7.0.2, PHP 8.2.31.

functions/editor.php lines 284 to 287:

protected static function my_gcd($a, $b) {
    $b = ( $a == 0 )? 0 : $b;
    return ( $a % $b )? self::my_gcd($b, abs($a - $b)) : $b;
}

When $a is 0, line 285 sets $b to 0 as well, and line 286 then evaluates
0 % 0. Under PHP 7 that emitted a warning and returned false, so the guard
worked by accident. Since PHP 8.0, modulo by zero throws DivisionByZeroError,
which is uncaught here and returns HTTP 500 to the REST request.

The zero reaches the function whenever an image's dimensions cannot be read, and
there is a common way for that to happen that has nothing to do with a user
deleting anything. When WordPress scales a large upload it keeps the
pre-scaling file alongside and records its name in the attachment metadata as
original_image. getCropData() unconditionally asks for that file at
functions/editor.php line 129:

$result['sourceImage']['original_image'] = $this->getUncroppedImageData($imagePostObj->ID, 'original_image');

and getUncroppedImageData() at lines 201 to 203 reads it without checking the
result:

if($imageSize === 'original_image') {
    $tmp = wp_getimagesize(wp_get_original_image_path($ID));
    $orig_img = [ wp_get_original_image_url($ID), $tmp[0], $tmp[1], false ];
}

If that pre-scaling file is gone — a migration, a media cleanup, a sync that
skipped it — wp_getimagesize() returns false, $tmp[0] and $tmp[1] are
null, and the fatal follows. The attachment itself can be perfectly healthy:
its main file and every generated size may still be on disk. Cropping is simply
impossible for it, and every attempt returns 500.

Note that this only reaches my_gcd() where the GMP extension is absent, since
gcd() prefers gmp_gcd() when it is available. Ours is absent, which is the
common case.

In #94 you wrote: "I can dimly remember that I designed the programming back then
(about 3 years ago) so that PHP would not crash when trying to crop the image with
the original uncropped image size."
That guard is the $b = ( $a == 0 )? 0 : $b;
line, and it did its job under PHP 7 — it just no longer can, because the very
operation it falls through to is now fatal rather than a warning. So this is not a
missing intention, only an intention that PHP 8 broke.

On our installation this affects 122 of the 4,500 attachments that carry a
scaled original: 2.7 per cent of them, and every one is a crop request that
cannot succeed.

Suggested fix, preserving the original intent that a zero input yields a zero
divisor result:

protected static function my_gcd($a, $b) {
    if ($a == 0 || $b == 0) {
        return 0;
    }
    return ( $a % $b )? self::my_gcd($b, abs($a - $b)) : $b;
}

A second suggestion, independent of the first: when the pre-scaling original is
missing but the scaled file is present, the plugin could fall back to the scaled
file rather than failing. The crop is then made from a smaller source, which is
worth saying in the interface, but it works. And where no file can be read at
all, a 404 with a message would serve the editor better than an exception
becoming a 500 — at present they see a failure with no explanation, and retrying
gives the same result.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions