imagefontwidth and imagefontheight fail on valid GDFont

jonaskohl commented 8 months ago

The following code:

<?php

const FONT_BUILTIN = 5;
const FONT_EXTERNAL = __DIR__ . "/MiscTT_12x20_LE.gdf";

// Uncomment to download sample font:
// copy("http://www.danceswithferrets.org/lab/gdfs/MiscTT_12x20_LE.gdf", FONT_EXTERNAL);
// Note: This is just a sample font. This problem occurs with every valid gdf file.

echo strval(imagefontwidth(FONT_BUILTIN)) . PHP_EOL;
echo strval(imagefontheight(FONT_BUILTIN)) . PHP_EOL;

$font = imageloadfont(FONT_EXTERNAL);
if ($font === false) die("Failed to load font" . PHP_EOL);
if (!($font instanceof GdFont)) die("Font is not of type " . GdFont::class . PHP_EOL);

var_dump($font);

$fontWidth = imagefontwidth($font);
$fontHeight = imagefontheight($font);

echo strval($fontWidth) . PHP_EOL;
echo strval($fontHeight) . PHP_EOL;

Resulted in this output:

9
15
object(GdFont)#1 (0) {
}
PHP Fatal error:  Uncaught TypeError: imagefontwidth(): Argument #1 ($font) must be of type GdFont|int, GdFont given in /Users/jonas/Desktop/sample.php:19
Stack trace:
#0 /Users/jonas/Desktop/sample.php(19): imagefontwidth(Object(GdFont))
#1 {main}
  thrown in /Users/jonas/Desktop/sample.php on line 19

Fatal error: Uncaught TypeError: imagefontwidth(): Argument #1 ($font) must be of type GdFont|int, GdFont given in /Users/jonas/Desktop/sample.php:19
Stack trace:
#0 /Users/jonas/Desktop/sample.php(19): imagefontwidth(Object(GdFont))
#1 {main}
  thrown in /Users/jonas/Desktop/sample.php on line 19

But I expected this output instead:

9
15
object(GdFont)#1 (0) {
}
12
20

The weird thing is that calls to imagestring work as expected. I'm also unable to reproduce this on non-mac systems. But the people over at MacPorts told me to report this here, so...

PHP Version

8.3.1

Operating System

macOS 14.2.1

nielsdos commented 8 months ago

Weird. Just a quick question: does this reproduce if you disable all extensions except gd?

jonaskohl commented 8 months ago

Yes, this also happens with everything except GD disabled

ryandesign commented 8 months ago

But the people over at MacPorts told me to report this here, so...

Right, I asked @jonaskohl to report it here because I could not reproduce the issue on an Intel MacBook Pro running macOS 12.7.2. I'm the maintainer of PHP in MacPorts and to my knowledge we're not doing anything weird in the way that we build PHP, so if there is something to be fixed, I expect it to be in PHP, not in MacPorts. For cross-referencing, here is the MacPorts bug report about this.

Jonas, what machine are you using? If an Apple Silicon Mac, that difference would be something to explore. For example, you could try running the Intel version of PHP in Rosetta 2 emulation and see if it has the same problem. To try that, you could sudo port install php83-gd +universal (which will build and install both Intel and Apple Silicon versions) and then you can try the Intel version with arch -x86_64 php83 sample.php.

ryandesign commented 8 months ago

Another thought: Could this be caused by some setting in your php.ini? I didn't see the problem when I had no php.ini or when it was a copy of php.ini-production or php.ini-development.

jonaskohl commented 8 months ago

I indeed use Apple Silicon (M3 Pro)!

I've tried both things (using the x86_64 version of php83-gd and also resetting my php.ini to php.ini-production) but the issue still appears.

nielsdos commented 8 months ago

So something must be overriding the class entry then somehow...

Can you try to compile PHP yourself, with the options: --enable-gd --enable-debug --enable-address-sanitizer --enable-undefined-sanitizer ?

jonaskohl commented 8 months ago

Huh, interesting... Building from source actually makes the problem disappear. The output is now the expected:

9
15
object(GdFont)#1 (0) {
}
12
20

./php -i output

nielsdos commented 8 months ago

Sounds like a build or configuration issue over at Macports then? Then again, even when you replaced the ini you got the issue. I don't have a mac, so I can't really debug this...

devnexen commented 8 months ago

worth noting it also happens with the homebrew package (all versions).

kocsismate commented 8 months ago

I also have an M3 Pro, and I managed to run the above test successfully. Of course, I built PHP from source so I agree that the issue is most likely with Macports.

ryandesign commented 8 months ago

I have now tested in a fresh MacPorts installation on a Mac mini with an M1 Apple Silicon processor running macOS 14.2.1 and I was able to reproduce the issue. This does not mean MacPorts is the problem; it means MacPorts used a different set of configure arguments, environment variables, and patches when it built php from source than the user did, and this difference is what exposes the problem, wherever it may be.

My first guess was optimization flags, and this guess seems to have been correct. Since 2013, MacPorts has built ports with the -Os optimization flag. If I rebuild php83-gd 8.3.1 with any other optimization flag (I tried -O0, -O1, -O2, and -O3) the problem does not occur. The optimization flags used to build php itself don't appear to influence the problem, only the flags used to build php83-gd. This system has Xcode 15.1 which has this version of Apple's fork of the clang compiler:

% clang --version
Apple clang version 15.0.0 (clang-1500.1.0.2.5)
Target: arm64-apple-darwin23.2.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Xcode 15.2 was released yesterday but its clang version is unchanged so I don't expect different results.

I then returned to my Intel MacBook Pro with macOS 12.7.2 where I had not previously observed the problem. I tried rebuilding php83-gd with various versions of llvm.org's clang compiler which I installed using MacPorts. The problem did not occur when php83-gd was compiled with clang 12.0.1 or 13.0.1 when using -Os but it did occur with clang 14.0.6, 15.0.7, 16.0.6, and 17.0.6 when using -Os.

I also checked php82-gd 8.2.14 and php81-gd 8.1.27 and they are also affected. php80-gd 8.0.30 is not affected. I would be interested to know what changed between 8.0.30 and 8.1.27 that causes this. I may test some more versions to try to find it, unless somebody already has an idea what it might be.

I don't have enough information at this point to say whether we have found a bug in clang or a bug in php.

ryandesign commented 8 months ago

worth noting it also happens with the homebrew package (all versions).

Homebrew has used -Os since 2012.

devnexen commented 8 months ago

Thanks for your input. it can be reproduced on linux too. tried with :

clang 17 and clang-git -> issue.
clang 13 -> no issue.

ryandesign commented 8 months ago

Thanks for the fix! I'll add it to MacPorts.

So the problem was uninitialized use of a variable. I'm surprised the compiler didn't warn about that, even if I add -Wall and -Wextra. It does finally warn if I add -Wconditional-uninitialized:

ext/gd/gd.c:2688:26: warning: variable 'font_obj' may be uninitialized when used here [-Wconditional-uninitialized]
        font = php_find_gd_font(font_obj, font_int);
                                ^~~~~~~~
ext/gd/gd.c:2680:23: note: initialize the variable 'font_obj' to silence this warning
        zend_object *font_obj;
                             ^
                              = NULL
ext/gd/gd.c:2688:36: warning: variable 'font_int' may be uninitialized when used here [-Wconditional-uninitialized]
        font = php_find_gd_font(font_obj, font_int);
                                          ^~~~~~~~
ext/gd/gd.c:2681:20: note: initialize the variable 'font_int' to silence this warning
        zend_long font_int;
                          ^
                           = 0

Maybe php should be using this warning flag by default. Using it exposes a lot of other instances of this as well, and I've only looked at gd, not the rest of php, though I don't know if the other potential uninitialized uses of variables are a problem or not.

I noticed you committed the fix to master and the 8.3 and 8.2 branches but not the 8.1 branch even though it is also affected. I realize php 8.1 is receiving security fixes only at this point. Is the use of an uninitialized variable that leads to undefined behavior not considered a security issue?

devnexen commented 8 months ago

I noticed you committed the fix to master and the 8.3 and 8.2 branches but not the 8.1 branch even though it is also affected. I realize php 8.1 is receiving security fixes only at this point. Is the use of an uninitialized variable that leads to undefined behavior not considered a security issue?

This is up to release managers, beside I do not know much macports but a patch could be written for 8.1 I think.

ryandesign commented 8 months ago

This is up to release managers,

Do I need to bring it to the attention of the release managers manually (if so, how?) or do they review fixes for potential backporting automatically?

beside I do not know much macports but a patch could be written for 8.1 I think.

Yes, I did add the patch for php 8.1, 8.2, and 8.3 in MacPorts. But I try to get patches incorporated upstream wherever possible since that way the fix reaches the largest possible audience and I don't have to maintain a patch forever.

devnexen commented 8 months ago

Do not get me wrong, I m not dismissing you :) I ve notified the proper persons but ultimately they will decide what to do. Thanks.

smalyshev commented 8 months ago

It indeed does not initialize the values if parameter parsing fails, but it should not get to php_find_gd_font if parameter parsing fails. So I don't really understand how initialization can be a problem there. Also, I am not sure I understand what this error means:

PHP Fatal error:  Uncaught TypeError: imagefontwidth(): Argument #1 ($font) must be of type GdFont|int, GdFont given in /Users/jonas/Desktop/sample.php:19

It needs GdFont and it got GdFont, so what's wrong there?

nielsdos commented 8 months ago

It needs GdFont and it got GdFont, so what's wrong there?

Looking at the assembly, the compiler removed a lot of code because the branching depended on an uninitialized value which is UB. So the assembly is bogus. In general we can't predict what a compiler is going to emit under such circumstances.

smalyshev commented 8 months ago

because the branching depended on an uninitialized value which is UB

Which branching though? The code isn't supposed to touch font_obj on parameter parse error, it's just supposed to return.

smalyshev commented 8 months ago

Tried to step through the compiled code with -Os and it definitely looks like some kind of compiler bug, because I am seeing this:

  * frame #0: 0x000000010028a6c0 cliphp`instanceof_function_slow(instance_ce=0x000000013ae1dc70, ce=0x000000013ae1dc70) at zend_operators.c:2464 [opt]
    frame #1: 0x00000001000ee394 cliphp`php_imagefontsize [inlined] instanceof_function(instance_ce=<unavailable>, ce=<unavailable>) at zend_operators.h:72:30 [opt]

Why is this wrong? Because instanceof_function_slow should never be called with the same arguments:

static zend_always_inline bool instanceof_function(
        const zend_class_entry *instance_ce, const zend_class_entry *ce) {
    return instance_ce == ce || instanceof_function_slow(instance_ce, ce);
}

and

    ZEND_ASSERT(instance_ce != ce && "Should have been checked already");

Note this has nothing to do with the font_ variables - it's all about function argument parsing yet. Indeed, if I enable that assertion, the reproduction code now ends up in:

Assertion failed: (instance_ce != ce), function instanceof_function_slow, file zend_operators.c, line 2465.

So the problem here is that the compiler somehow killed the instance_ce == ce clause - which, again, does not deal with any undefined variables at all.

smalyshev commented 8 months ago

For illustration, this is the correct assembly code for it:

    0x1000ee358 <+64>:  cmp    w8, #0x8                  ;; <= this checks for IS_OBJECT 
    0x1000ee35c <+68>:  b.ne   0x1000ee400               ; <+232> [inlined] zend_parse_arg_obj_or_long + 4 at zend_API.h:2413:13
    0x1000ee360 <+72>:  ldr    x1, [x23, #0x770]
    0x1000ee364 <+76>:  ldr    x0, [x21]
    0x1000ee368 <+80>:  ldr    x8, [x0, #0x10]
    0x1000ee36c <+84>:  cmp    x8, x1                    ;; <= this compares classes
    0x1000ee370 <+88>:  b.eq   0x1000ee38c               ; <+116> at zend_API.h
    0x1000ee374 <+92>:  mov    x0, x8
    0x1000ee378 <+96>:  bl     0x10028a6c0               ; instanceof_function_slow at zend_operators.c:2464
    0x1000ee37c <+100>: cbz    w0, 0x1000ee3fc           ; <+228> [inlined] zval_get_type at zend_types.h:648:18

and this is the buggy one:

    0x1000ee378 <+48>:  cmp    w8, #0x8                  ;; <= this checks for IS_OBJECT 
    0x1000ee37c <+52>:  b.ne   0x1000ee398               ; <+80> at gd.c
    0x1000ee380 <+56>:  adrp   x8, 3675
    0x1000ee384 <+60>:  ldr    x1, [x8, #0x770]
    0x1000ee388 <+64>:  ldr    x8, [x21, #0x50]
    0x1000ee38c <+68>:  ldr    x0, [x8, #0x10]
    0x1000ee390 <+72>:  bl     0x10028a6c0               ; instanceof_function_slow at zend_operators.c:2464

Note how class comparison is missing and it's going directly for instanceof_function_slow - which obviously will fail because it's not supposed to get this type of arguments.

ranvis commented 8 months ago

@smalyshev The cause of the undefined behavior that I could explain is the expression php_find_gd_font(font_obj, font_int). Before the fix, the pushed variable font_int is uninitialized when font_obj is set to non-null by zend_parse_arg_obj_or_long().

SakiTakamachi commented 8 months ago

I don't know if this is related because I haven't looked closely, but it seems like the test is failing on Travis.

https://app.travis-ci.com/github/php/php-src/builds/268436286#L2882

devnexen commented 8 months ago

The sample file used for the test is endian dependant.

php / php-src

imagefontwidth and imagefontheight fail on valid GDFont #13082

PHP Version

Operating System