Fix for bug #77184 #3673

cmbasnett · 2018-11-21T21:42:48Z

nikic · 2018-11-21T22:06:19Z

The fix looks good to me. It would be good to also add a test with a reduced image. Not sure if there's a tool to only keep the exif metadata and discard the image data. Maybe @KalleZ knows?

KalleZ · 2018-11-22T08:00:04Z

@nikic The ExifTool[1] allows such, in the past I have created a 1x1 JPG, erased the meta data that the editor I made it with may have added and propogated it with relevant test data using -tagsFromFile or similar options.

[1] https://www.sno.phy.queensu.ca/~phil/exiftool/

cmbasnett · 2018-11-22T19:56:43Z

I'll put a test together and amend the PR.

cmbasnett · 2018-11-22T22:31:39Z

I'm trying to compile PHP from source on my MacOS Mojave, but am unable to do it

When running ./configure with the following command line:

./configure --without-sqlite3 --with-iconv=/usr/local/opt/libiconv --without-sqlite

I keep getting this error:

...
checking for PDO_DBLIB support via FreeTDS... no
checking for Firebird support for PDO... no
checking for MySQL support for PDO... no
checking Oracle OCI support for PDO... no
checking for ODBC v3 support for PDO... no
checking for PostgreSQL support for PDO... no
checking for sqlite 3 support for PDO... yes
checking for sqlite3 files in default path... not found
configure: error: Please reinstall the sqlite3 distribution

I don't know why it's searching for sqlite3 when I explicitly set --without-sqlite3 in the command line. Can someone help me out here?

P.S. I have actually already installed sqlite3 via Homebrew and the bin directory is in my $PATH, so this is even more of a head-scratcher.

nikic · 2018-11-22T22:35:41Z

@cmbasnett sqlite and PDO sqlite are separate extensions, so something like --without-pdo-sqlite should probably do it.

KalleZ · 2018-11-22T22:44:45Z

@cmbasnett alternatively you can do --disable-all --enable-cli for minimal builds thats usually good enough for testing things like that :)

Edit: For ext/exif, you will also need ext/mbstring so something like: ./configure --disable-all --enable-cli --enable-exif --enable-mbstring should do

cmbasnett · 2018-11-22T23:04:04Z

Alright that got me to the point where I can actually run make, but now I'm getting this error:

/bin/sh /Users/colinbasnett/php-src/libtool --silent --preserve-dup-deps --mode=compile cc -DZEND_ENABLE_STATIC_TSRMLS_CACHE=1 -Iext/standard/ -I/Users/colinbasnett/php-src/ext/standard/ -DPHP_ATOM_INC -I/Users/colinbasnett/php-src/include -I/Users/colinbasnett/php-src/main -I/Users/colinbasnett/php-src -I/Users/colinbasnett/php-src/ext/date/lib -I/Users/colinbasnett/php-src/ext/mbstring/oniguruma -I/Users/colinbasnett/php-src/ext/mbstring/libmbfl -I/Users/colinbasnett/php-src/ext/mbstring/libmbfl/mbfl -I/Users/colinbasnett/php-src/TSRM -I/Users/colinbasnett/php-src/Zend  -no-cpp-precomp  -g -O2 -fvisibility=hidden -DZEND_SIGNALS   -c /Users/colinbasnett/php-src/ext/standard/url_scanner_ex.c -o ext/standard/url_scanner_ex.lo
clang: error: no such file or directory: '/Users/colinbasnett/php-src/ext/standard/url_scanner_ex.c'
clang: error: no input files

I had a look, and indeed, url_scanner_ex.c does not exist.

KalleZ · 2018-11-22T23:08:47Z

@cmbasnett That file is one of those auto-generated by RE2C, not sure if doing a new clean build will fix it and regenerate it as I only develop on Windows, perhaps @nikic knows how it works there, for me usually I end up doing this on Windows:

rd /s /q x64 && nmake clean && buildconf && config.nice && nmake
(Remove build directory, clean remaining build files, rebuild configure, re-use last configure and make)

Alternatively try touch the scanner files and see if RE2C picks it up, but again, I'm not familar with the Unix build system at all, I'm afraid.

nikic · 2018-11-22T23:11:19Z

It might be that you don't have re2c installed at all. It recently became a required dependency for builds from git and I suspect we might not have the necessary checks in place to ensure it's available yet (cc @petk).

KalleZ · 2018-11-22T23:12:43Z

@nikic I thought it was a requirement for source builds of PHP on Unix? It's been on Windows since 5.3.0 when we moved from Flex to RE2C

nikic · 2018-11-22T23:15:40Z

@KalleZ Yeah, I was referring to @cmbasnett's issue on macos here. On Unix/BSD you can call ./genfiles to force a regeneration of all parsers and lexers, but as said, in this case I suspect that the issue is just re2c not being installed at all.

KalleZ · 2018-11-22T23:17:17Z

@nikic Ah right, I just expected ./configure to bail if RE2C was not found before starting to parse arguments, thats why I was left wondering

petk · 2018-11-22T23:24:49Z

The re2c is missing yes and also additionally you'll need bison probably.

Currently check for re2c raises only warning: https://github.com/php/php-src/blob/master/acinclude.m4#L2100
To throw error and stop the configure script probably something could be done here yes...

cmbasnett · 2018-11-22T23:42:43Z

Alright I got it compiled and I can run tests (I installed re2c and did the build clean steps you recommended). In order to make a tiny test image file, I ran EXIFTool to transfer the metadata to a 1x1 using tagsFromFile, but it seems like exiftool is actually reinterpreting the original values in the tags and "simplifying" them, so the numerator and denominator are different and therefore don't reproduce the bug. I'll do some poking around and see if I can just gut the pixel data and leave the EXIF intact somehow.

KalleZ · 2018-11-22T23:47:06Z

@cmbasnett Have you looked at the ExifTool commands? There is a fair few to define I/O formatting[1], mainly looking at -coordFormat, but it has been quite a long time since I did this and atm I don't have access to my old scripts for these convertions I did, but I do remember encountering something similar.

[1] https://www.sno.phy.queensu.ca/~phil/exiftool/exiftool_pod.html

cmbasnett · 2018-11-23T01:16:33Z

Very bizarre, the test passes on my machine:

Perhaps this is some sort of issue with line-endings or a text-encoding error (there are some non-standard characters in the EXIF data)?

TEST_PHP_EXECUTABLE=sapi/cli/php \
sapi/cli/php run-tests.php ext/exif/tests/bug77184

=====================================================================
PHP         : sapi/cli/php
PHP_SAPI    : cli
PHP_VERSION : 7.4.0-dev
ZEND_VERSION: 3.4.0-dev
PHP_OS      : Darwin - Darwin Colins-MBP.spexi.local 18.2.0 Darwin Kernel Version 18.2.0: Fri Oct  5 19:41:49 PDT 2018; root:xnu-4903.221.2~2/RELEASE_X86_64 x86_64
INI actual  : /Users/colinbasnett/php-src
More .INIs  :
---------------------------------------------------------------------
PHP         : /Users/colinbasnett/php-src/sapi/phpdbg/phpdbg
PHP_SAPI    : phpdbg
PHP_VERSION : 7.4.0-dev
ZEND_VERSION: 3.4.0-dev
PHP_OS      : Darwin - Darwin Colins-MBP.spexi.local 18.2.0 Darwin Kernel Version 18.2.0: Fri Oct  5 19:41:49 PDT 2018; root:xnu-4903.221.2~2/RELEASE_X86_64 x86_64
INI actual  : /Users/colinbasnett/php-src
More .INIs  :
---------------------------------------------------------------------
CWD         : /Users/colinbasnett/php-src
Extra dirs  :
VALGRIND    : Not used
=====================================================================
Running selected tests.
PASS Bug #74428 (Unsigned rational numbers are written out as signed rationals) [ext/exif/tests/bug77184/bug77184.phpt]
=====================================================================
Number of tests :    1                 1
Tests skipped   :    0 (  0.0%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    0 (  0.0%) (  0.0%)
Expected fail   :    0 (  0.0%) (  0.0%)
Tests passed    :    1 (100.0%) (100.0%)
---------------------------------------------------------------------
Time taken      :    0 seconds
=====================================================================

KalleZ · 2018-11-23T03:10:50Z

Testing it locally, I can confirm your fix works, however what causes the fail is that the FileDateTime value of the returned array is different (its allocated to the st_mtime member of the stat struct), which should simply be:

int(1542931229)
->
int(%d)

Similar to what other tests does, for example ext/exif/tests/bug34704.phpt.

Edit: Alternatively, you can choose to simply just dump the value you are testing, like:

$tags = exif_read_data(...);

echo $tags['GPSLatitude'][2], PHP_EOL;
echo $tags['GPSLongitude'][2], PHP_EOL;

(Nitpick: The .phpt file is marked as binary in your commit, it should just be text/plain)

cmbasnett · 2018-11-23T19:41:44Z

@KalleZ I was looking at the repo's .gitattributes file and it looks like it's not forcing .phpt files to be interpreted as text. The file originally had some weird bytes in it, so it must have interpreted it as a binary file. Would the fix for this be to force the text attribute on *.phpt files? I understand this would be a global change, but aside from adding a local gitattributes file in the folder I don't know how else to force the text attribute.

KalleZ · 2018-11-23T20:25:26Z

@cmbasnett Actually thats a good point, I think the .gitattributes file is better left as is and then converting the .phpt file here to text. (cc @petk)

All in all, I think the PR is good enough to be merged :)

nikic · 2018-11-24T00:00:53Z

ext/exif/tests/bug77184/bug77184.phpt

+<?php if (!extension_loaded('exif')) print 'skip exif extension not available';?>
+--INI--
+output_handler=
+zlib.output_compression=0


I realize these are also present in some other exit test cases -- does someone know why? They don't look immediately exif related to me.

I was just cargo-culting those into the test since I assumed they were important; I'm unfamiliar with the PHP testing ecosystem.

@nikic I'm pretty sure they are a leftover from a copy-paste from another test. I guess they would origin from some test that was printing binary data or similar to compare it with something else. But they should most likely be removed, but thats something we can do in a separate commit after this PR

Yeah, I agree that we should handle that separately.

nikic · 2018-11-26T21:51:06Z

Merged as 9ec519e into PHP 7.2+.

Thanks for fixing this and going through the trouble of creating a test case!

Fix for bug https://bugs.php.net/bug.php?id=77184

371285d

nikic changed the title ~~Fix for bug https://bugs.php.net/bug.php?id=77184~~ Fix for bug #77184 Nov 21, 2018

petk added the Bug label Nov 21, 2018

nikic added the Waiting on Author label Nov 22, 2018

petk mentioned this pull request Nov 22, 2018

Add re2c checking with error exit code #3676

Closed

Added test to cover bug #77184.

643d9e3

Fix for test to simply test the GPS values instead of the whole file.

26d6576

nikic removed the Waiting on Author label Nov 23, 2018

nikic reviewed Nov 24, 2018

View reviewed changes

nikic approved these changes Nov 26, 2018

View reviewed changes

nikic closed this Nov 26, 2018

Fix for bug #77184 #3673

Fix for bug #77184 #3673

Uh oh!

Conversation

cmbasnett commented Nov 21, 2018 • edited by nikic Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Nov 21, 2018

Uh oh!

KalleZ commented Nov 22, 2018

Uh oh!

cmbasnett commented Nov 22, 2018

Uh oh!

cmbasnett commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Nov 22, 2018

Uh oh!

KalleZ commented Nov 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmbasnett commented Nov 22, 2018

Uh oh!

KalleZ commented Nov 22, 2018

Uh oh!

nikic commented Nov 22, 2018

Uh oh!

KalleZ commented Nov 22, 2018

Uh oh!

nikic commented Nov 22, 2018

Uh oh!

KalleZ commented Nov 22, 2018

Uh oh!

petk commented Nov 22, 2018

Uh oh!

cmbasnett commented Nov 22, 2018

Uh oh!

KalleZ commented Nov 22, 2018

Uh oh!

cmbasnett commented Nov 23, 2018

Uh oh!

KalleZ commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmbasnett commented Nov 23, 2018

Uh oh!

KalleZ commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic Nov 24, 2018

Choose a reason for hiding this comment

Uh oh!

cmbasnett Nov 24, 2018

Choose a reason for hiding this comment

Uh oh!

KalleZ Nov 24, 2018

Choose a reason for hiding this comment

Uh oh!

nikic Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

nikic commented Nov 26, 2018

Uh oh!

Uh oh!

cmbasnett commented Nov 21, 2018 •

edited by nikic

Loading

cmbasnett commented Nov 22, 2018 •

edited

Loading

KalleZ commented Nov 22, 2018 •

edited

Loading

KalleZ commented Nov 23, 2018 •

edited

Loading

KalleZ commented Nov 23, 2018 •

edited

Loading