Fix tests on 32-bit Windows OS #8448

mvorisek · 2022-04-27T20:45:48Z

discovered during GH-8392 devel

see comments for the decisions

mvorisek · 2022-04-27T20:46:07Z

ext/gd/tests/bug77269.phpt

@@ -1,18 +0,0 @@
--TEST--
-Bug #77269 (Potential unsigned underflow in gdImageScale)


duplicate of ext/gd/tests/bug77272.phpt

Well, not exactly (this test is supposed to work with pretty old GD, but the other only for somewhat recent versions), but I think it's okay to drop bug77269.phpt now, but for "master" only.

ext/sysvshm/tests/bug72858.phpt

mvorisek · 2022-04-27T20:47:07Z

sapi/phpdbg/tests/watch_001.phpt

@@ -2,9 +2,6 @@
 Test simple recursive watchpoint
 --SKIPIF--
 <?php
-if (PHP_INT_SIZE == 4) {


Probably fixed, tests pass:

Test simple recursive watchpoint [D:\a\php-src\php-src\sapi\phpdbg\tests\watch_001.phpt] (warn: XFAIL section but test passes) Test simple array watchpoint with replace [D:\a\php-src\php-src\sapi\phpdbg\tests\watch_002.phpt] (warn: XFAIL section but test passes) Test simple watchpoint with replace [D:\a\php-src\php-src\sapi\phpdbg\tests\watch_003.phpt] (warn: XFAIL section but test passes) Test detection of inline string manipulations on zval watch [D:\a\php-src\php-src\sapi\phpdbg\tests\watch_004.phpt] (warn: XFAIL section but test passes)

The issues may be solved, but I'd only remove that skipif clause for "master".

if it passes for PHP-8.0, why do not fix this good behaviour for PHP-8.0 too?

It may pass, but would it pass in all enviroments? I mean the skip reason is "There may be flaws in the implementation of watchpoints that cause failures". This may not have been resolved generally. Maybe @krakjoe can comment on this?

cmb69 · 2022-04-28T09:16:56Z

ext/gd/tests/bug77269.phpt

@@ -1,18 +0,0 @@
--TEST--
-Bug #77269 (Potential unsigned underflow in gdImageScale)


Well, not exactly (this test is supposed to work with pretty old GD, but the other only for somewhat recent versions), but I think it's okay to drop bug77269.phpt now, but for "master" only.

cmb69 · 2022-04-28T09:43:17Z

ext/gd/tests/bug77272.phpt

@@ -5,6 +5,7 @@ memory_limit=-1
 --SKIPIF--
 <?php
 if (!extension_loaded('gd')) die('skip gd extension not available');
+if (PHP_INT_SIZE != 8) die("skip this test is for 64bit platform only");


Doesn't this test succeed on 32bit Linux?

2**28 is still within the 32-bit integer range, I need to investigate further

That is correct, but there are additional allocations, and I think that the scaling uses a truecolor image which requires 4 byte for each pixel. So we're at 2^30 (plus), what usually doesn't work on Windows (usually max. 2 GiB per process), while Linux may allow more.

ext/sysvshm/tests/bug72858.phpt

cmb69 · 2022-04-28T10:00:23Z

sapi/phpdbg/tests/watch_001.phpt

@@ -2,9 +2,6 @@
 Test simple recursive watchpoint
 --SKIPIF--
 <?php
-if (PHP_INT_SIZE == 4) {


The issues may be solved, but I'd only remove that skipif clause for "master".

Girgias · 2022-05-04T09:41:50Z

Please wait for #8392 to be merged before working on this, also CI enabling of x86 testing for Windows should be either part of this PR or a follow-up instead of polluting the other one which should just be a change from Appveyor to GitHub

mvorisek · 2022-05-09T09:03:52Z

@cmb69 can you please review the test changes?

I tested them on Windows x64 with 32 bit php, but I have no access to 32 bit linux nor 32 bit Windows machines.

mvorisek · 2022-05-16T15:30:49Z

@cmb69 I propose all changes made, it fixes 32b build on Win x64 and I have no access to 32b machines. I propose to merge as is, if there is any issue with rate 32b OS combination, then we can add XFAIL/SKIP back, but I belive the issues were already skipped and better to assert no issues in CI if there are no issues known

cmb69

I'm fine with merging the changes to the phpdbg tests into master, and also dropping bug77269.phpt there. I don't think we shouldn't touch bug77272.phpt without closer investigation. The changes to bug72858.phpt defeat the purpose of that test.

mvorisek · 2022-05-16T16:14:09Z

phpdbg - will you cherrypick during merge?
gd bug77269/bug77272, discussed in Fix tests on 32-bit Windows OS #8448 (comment) - what exactly? I belive the expectation for gd lib < 2.2.5 will not be satisfied by the other test, except different memory limit, there are the same
bug72858 - discussed in Fix tests on 32-bit Windows OS #8448 (comment) - the test was NOT skipped on 32b build, but if such build is tested, the test fails, so it needs change, or please advise how to fix the test or php-src

cmb69 · 2022-05-16T16:23:51Z

bug77272.phpt should run regardless of the architecture, unless it is clear, that it never can pass under 32bit OS (I still believe it passes on Linux).

bug72858.phpt is supposed to run under 32bit only, to trigger shm_attach() failure. I have yet to see that test failing on any 32bit architecture.

mvorisek · 2022-05-16T16:35:15Z

bug77272.phpt should run regardless of the architecture, unless it is clear, that it never can pass under 32bit OS (I still believe it passes on Linux).

it does not pass /w 32b build for Windows, can you reproduce?

bug72858.phpt is supposed to run under 32bit only, to trigger shm_attach() failure. I have yet to see that test failing on any 32bit architecture.

on 32b build, there is no warning, if the test is failing on native 32b architecture/host, how to fix it?

mvorisek · 2022-05-26T20:39:53Z

@cmb69 let's merge this as is, all changes are needed to pass tests when 32-bit php is built

and please revert c344d30 to master as the skips for 32-bit builds are not needed

cmb69

bug72858.phpt is supposed to run under 32bit only, to trigger shm_attach() failure. I have yet to see that test failing on any 32bit architecture.

So why did you delete that test?

mvorisek · 2022-05-27T11:51:06Z

@cmb69 because #8448 (comment)

cmb69 · 2022-05-27T12:02:04Z

Nikita wrote "If this test no longer works on 32-bit,", but for me it works every single time I run it. I still have yet to see a single case where it fails.

mvorisek · 2022-05-27T12:06:30Z

Nikita wrote "If this test no longer works on 32-bit,", but for me it works every single time I run it. I still have yet to see a single case where it fails.

for me, the original expected warning is not present when I compile PHP for x86/32b (and for x64/64b, the test is skipped)

(all tests done on 64 bit host OS)

mvorisek · 2022-05-27T12:09:55Z

proof: https://github.com/php/php-src/runs/6289390413?check_suite_focus=true#step:8:3720

cmb69 · 2022-05-27T13:22:18Z

Okay, thanks. https://github.com/cmb69/php-ftw/actions/runs/2396415661 confirms that. I'll have a closer look.

cmb69 · 2022-05-27T14:20:54Z

I still think that the test is useful, but the actual $size is not; it is only important that failure to map is caught. So I thought that using PHP_INT_MAX might be a nice opportunity to extend the test to 64bit architectures. There I'd get:

Warning: shm_attach(): Failed for key 0x64: No error in %s on line %d

Task failed successfully! ;)

This reverts commit 91dca4f.

mvorisek · 2022-05-27T14:32:27Z

I still think that the test is useful, but the actual $size is not; it is only important that failure to map is caught. So I thought that using PHP_INT_MAX might be a nice opportunity to extend the test to 64bit architectures. There I'd get:
Warning: shm_attach(): Failed for key 0x64: No error in %s on line %d
Task failed successfully! ;)

updated, but are you sure it will fail on systems with free memory much larger than PHP_INT_MAX? like when 50 GB is available and the PHP_INT_MAX is 2 GB when build for x86/32b.

mvorisek · 2022-10-12T13:50:22Z

bug77272.phpt should run regardless of the architecture, unless it is clear, that it never can pass under 32bit OS (I still believe it passes on Linux).

you are right, the test should pass, there is some detection in gd overflow2 function (for some reasons, there are almost two identical copies for this function)

do you have any idea how to fix the gd lib?

Bug #77269 (Potential unsigned underflow in gdImageScale) [D:\a\php-src\php-src\ext\gd\tests\bug77269.phpt]
Bug #77272 (imagescale() may return image resource on failure) [D:\a\php-src\php-src\ext\gd\tests\bug77272.phpt]

tests are failing consistently on x86 build

cmb69 · 2022-10-12T14:33:48Z

in gd overflow2 function (for some reasons, there are almost two identical copies for this function)

Ugh, what a mess! The implementation in gd_security.c is used for bundled libgd; the implementation in gd_compat.c might be used for external libgd, although we require libgd >= 2.1.0, which always should have the implementation in gd_security.c. I guess gd_compat.c is superfluous for years. I'm gonna check this out.

Bug #77269 (Potential unsigned underflow in gdImageScale) [D:\a\php-src\php-src\ext\gd\tests\bug77269.phpt]

Like I said above, this test should be removed for "master".

Bug #77272 (imagescale() may return image resource on failure) [D:\a\php-src\php-src\ext\gd\tests\bug77272.phpt]

I suggest to skip this test on 32bit Windows.

cmb69 · 2022-10-12T15:24:56Z

I guess gd_compat.c is superfluous for years. I'm gonna check this out.

Unfortunately, overflow2 is not exported by libgd, and we call it from imageloadfont(). I'm not sure yet whether we should replace these overflow2() calls with something else, or whether I should suggest to deprecate the whole gdfont stuff (which is about primitive pixel fonts, not TTF).

mvorisek commented Apr 27, 2022

View reviewed changes

ext/sysvshm/tests/bug72858.phpt Show resolved Hide resolved

mvorisek commented Apr 27, 2022

View reviewed changes

mvorisek force-pushed the fix_32bit_testing branch 2 times, most recently from 7106c77 to a06e8d8 Compare April 27, 2022 21:48

mvorisek marked this pull request as draft April 27, 2022 23:16

mvorisek marked this pull request as ready for review April 28, 2022 00:07

cmb69 reviewed Apr 28, 2022

View reviewed changes

mvorisek marked this pull request as draft April 28, 2022 14:05

mvorisek force-pushed the fix_32bit_testing branch from a06e8d8 to 6b2dbb8 Compare May 4, 2022 07:28

mvorisek mentioned this pull request May 4, 2022

Test Windows x86 and x64 with Github Actions #8392

Closed

mvorisek force-pushed the fix_32bit_testing branch from 6ed2373 to cf3f55d Compare May 4, 2022 12:17

Fix tests on 32-bit Windows OS

978f214

mvorisek force-pushed the fix_32bit_testing branch from cf3f55d to 978f214 Compare May 4, 2022 13:54

mvorisek marked this pull request as ready for review May 4, 2022 13:54

cmb69 requested changes May 16, 2022

View reviewed changes

rm xfail for master only

c344d30

ramsey added Category: Tests OS: Windows labels May 22, 2022

Delete bug72858.phpt

91dca4f

cmb69 self-assigned this May 27, 2022

cmb69 requested changes May 27, 2022

View reviewed changes

Revert "Delete bug72858.phpt"

36304a3

This reverts commit 91dca4f.

test shm_attach /w PHP_INT_MAX

80f22ab

mvorisek force-pushed the fix_32bit_testing branch from 21bc6ad to 80f22ab Compare May 27, 2022 17:32

cmb69 closed this in c756e97 Jun 29, 2022

mvorisek deleted the fix_32bit_testing branch June 29, 2022 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tests on 32-bit Windows OS #8448

Fix tests on 32-bit Windows OS #8448

mvorisek commented Apr 27, 2022

mvorisek Apr 27, 2022

cmb69 Apr 28, 2022

mvorisek Apr 27, 2022 •

edited

Loading

cmb69 Apr 28, 2022

mvorisek Apr 28, 2022 •

edited

Loading

cmb69 Apr 28, 2022

cmb69 Apr 28, 2022

cmb69 Apr 28, 2022

mvorisek Apr 28, 2022

cmb69 Apr 28, 2022

cmb69 Apr 28, 2022

Girgias commented May 4, 2022

mvorisek commented May 9, 2022

mvorisek commented May 16, 2022

cmb69 left a comment

mvorisek commented May 16, 2022

cmb69 commented May 16, 2022

mvorisek commented May 16, 2022

mvorisek commented May 26, 2022 •

edited

Loading

cmb69 left a comment

mvorisek commented May 27, 2022

cmb69 commented May 27, 2022

mvorisek commented May 27, 2022

mvorisek commented May 27, 2022

cmb69 commented May 27, 2022

cmb69 commented May 27, 2022

mvorisek commented May 27, 2022 •

edited

Loading

mvorisek commented Oct 12, 2022

cmb69 commented Oct 12, 2022

cmb69 commented Oct 12, 2022

		@@ -1,18 +0,0 @@
		--TEST--
		Bug #77269 (Potential unsigned underflow in gdImageScale)

Fix tests on 32-bit Windows OS #8448

Fix tests on 32-bit Windows OS #8448

Conversation

mvorisek commented Apr 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvorisek Apr 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvorisek Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Girgias commented May 4, 2022

mvorisek commented May 9, 2022

mvorisek commented May 16, 2022

cmb69 left a comment

Choose a reason for hiding this comment

mvorisek commented May 16, 2022

cmb69 commented May 16, 2022

mvorisek commented May 16, 2022

mvorisek commented May 26, 2022 • edited Loading

cmb69 left a comment

Choose a reason for hiding this comment

mvorisek commented May 27, 2022

cmb69 commented May 27, 2022

mvorisek commented May 27, 2022

mvorisek commented May 27, 2022

cmb69 commented May 27, 2022

cmb69 commented May 27, 2022

mvorisek commented May 27, 2022 • edited Loading

mvorisek commented Oct 12, 2022

cmb69 commented Oct 12, 2022

cmb69 commented Oct 12, 2022

mvorisek Apr 27, 2022 •

edited

Loading

mvorisek Apr 28, 2022 •

edited

Loading

mvorisek commented May 26, 2022 •

edited

Loading

mvorisek commented May 27, 2022 •

edited

Loading