Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exif crash on unknown encoding was fixed #293

Closed
wants to merge 5 commits into from

Conversation

Draal
Copy link

@Draal Draal commented Mar 3, 2013

zend_multibyte_encoding_converter returns size_t type
size_t is unsigned integer, so condition <0 is false on dummy_encoding_converter ((size_t)-1) and exif_process_unicode returns unfilled xp_field->size

@weltling
Copy link
Contributor

weltling commented Mar 4, 2013

Good catch! Please add also some comment as it should be touched anyway once encoding_converter isn't dummy anymore. Also a test should be there ;)

@Draal
Copy link
Author

Draal commented Mar 4, 2013

I don't quite catch what comment you mean. Could you, please? tell me where I can find information about the rules of making PHP tests? Thanks

@weltling
Copy link
Contributor

weltling commented Mar 4, 2013

I meant just to put a small comment near your change like /* XXX this will fail again if encoding_converter delivers a real length */. Because once (and if) encoding_converter isn't dummy anymore, it'd deliver some real count of bytes and not always (size_t)-1. In that case the code you've fixed would fail again but with a comment it'd be easier to figure out.

There are some docs about the test file sections here http://qa.php.net/phpt_details.php . Also in each ext dir is a 'tests' folder where the tests live, like in your case ext/exif/tests/ , so you can look at those and do yours by example.

Comments added
All bugs with zend_multibyte_encoding_converter was fixed in exif.
@Draal
Copy link
Author

Draal commented Mar 5, 2013

"Because once (and if) encoding_converter isn't dummy anymore, it'd deliver some real count of bytes and not always (size_t)-1" - but if it returns real size, it fills &len?

@weltling
Copy link
Contributor

weltling commented Mar 5, 2013

Exactly, but the len isn't always SIZE_MAX )

@Draal
Copy link
Author

Draal commented Mar 5, 2013

ok.
Is pull request ok now?

@weltling
Copy link
Contributor

weltling commented Mar 5, 2013

Ah, now I see what you mean

/* XXX this will fail again if encoding_converter delivers a real length and doesn't fill len */

How do you come to "and doesn't fill len"? :)

@Draal
Copy link
Author

Draal commented Mar 5, 2013

I changed comments :)

@laruence
Copy link
Member

laruence commented Mar 5, 2013

hey, is this change also related to https://bugs.php.net/bug.php?id=62523 ?

thanks

@weltling
Copy link
Contributor

weltling commented Mar 5, 2013

@laruence looks not like that, this one is related to zend_multibyte_encoding_converter which starts to be present in 5.4 and is still a dummy in 5.5. But in the ticket it's mentioned to be reproduceable in 5.4, too. And the code path used looks similar.
@Draal I'll test this PR asap. Could you please check with the data from #62523 whether your fix works? May be adding them to the tests.

@laruence
Copy link
Member

laruence commented Mar 5, 2013

@weltling thanks :)

@Draal
Copy link
Author

Draal commented Mar 5, 2013

This fix doesn't related to #62523

@weltling
Copy link
Contributor

weltling commented Mar 5, 2013

Can confirm that, sadly. Files from #62523 still fail with this PR. However some similarities are present. Here we deal with the zend encoding converter, in #62523 it's the same with mbstring. Not sure, shouldn't mbstring override zend handlers (if present)? I have to test that when I've more time.

@Draal I have your test not passing on windows with vc9, here's the test diff

http://belski.net/phpz/pulls/draal/exif_encoding_crash.out

Most of the array keys look plausible, but there are some having binary data in it, namely ComponentsConfiguration, FileSource, SceneType, CFAPattern, UndefinedTag:0xEA1C. Sure those contain what they should?

Had no chance yet to test on Linux, will do as soon as I got my hands there.

@weltling
Copy link
Contributor

weltling commented Mar 5, 2013

And is that out what you see on your side?

@Draal
Copy link
Author

Draal commented Mar 6, 2013

On linux is the same result. I think it is normal, because test crash image is a specially formed image (the original had 3MB size).
On other image for example "php exif_read_exif_data_basic.phpt" it's okey.

@weltling
Copy link
Contributor

weltling commented Mar 6, 2013

So that's fine, the most important sections are correct, the others seem to be at least ok with their specs when I look here

http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/EXIF.html

Where could they come from is completely another story :)

So please update the EXPECT section with that array, as now you expect bool(false) there and the test fails.

@Draal
Copy link
Author

Draal commented Mar 6, 2013

I changed the test to check only php crash.

@Draal
Copy link
Author

Draal commented Mar 11, 2013

Is it ok now? Can I close pull request?

@weltling
Copy link
Contributor

Please let it ripen, may be someone else could take a look. The PR will be closed after merge

@ghost ghost assigned dsp Mar 19, 2013
@smalyshev
Copy link
Contributor

@Draal I am unable to reproduce the bug even with unpatched version with this test. The function exif_process_user_comment doesn't seem to even be called there, and ImageInfo.UserComment is NULL when parsing the test image. Does the test work for you?

@Draal
Copy link
Author

Draal commented Apr 1, 2013

This test image crash on exif_process_unicode,
the changes in exif_process_user_comment was written by logic, test image doesn't pass into it.

@smalyshev
Copy link
Contributor

@Draal still, I am unable to reproduce the problem with this test and unpatched code. Are there any special conditions or requirements for it to reproduce?

@Draal
Copy link
Author

Draal commented Apr 2, 2013

it is strange.
I builded php on linux (Centos, gcc Version 4.4.6 20120305 (Red Hat 4.4.6-4) (GCC)) from PHP-5.4.13 branch and ran it. Do you try reproduce bug on linux?

[draal@dev <exif_crash_fix>~/src/php-src/ext/exif/tests]$ /home/draal/tmp/php exif_encoding_crash.phpt
--TEST--
PHP crash when zend_multibyte_encoding_converter returns (size_t)-1)
--SKIPIF--
--FILE--
Ошибка сегментирования

@Draal
Copy link
Author

Draal commented Apr 2, 2013

Ошибка сегментирования = Segmentation fault

@smalyshev
Copy link
Contributor

@Draal doesn't happen for me on Linux either:
Linux ... 2.6.32-279.el6.x86_64 #1 SMP Fri Jun 22 12:19:21 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
No segfault with the image attached, also for me the encoding function doesn't actually return -1. Maybe some special settings are needed?

@Draal
Copy link
Author

Draal commented Apr 7, 2013

@smalyshev This bug reproduce only on 32bit platform (But it is a platform independent bug in general).
xp_field->size is zero on 64bit platform (I don't know why. May be on 64bit start memory was filled by zero?):
Breakpoint 1, exif_process_unicode (ImageInfo=0x7fffffffa200, xp_field=0x7ffff1f42e10, tag=40092, szValuePtr=0x7ffff7fe5d90 " ", ByteCount=74)
at /var/src/php-src/ext/exif/exif.c:2702
2702 xp_field->tag = tag;
(gdb) p xp_field->size
$7 = 0

xp_field->size is any on 32bit platform :
Breakpoint 1, exif_process_unicode (ImageInfo=0xbfffbd38, xp_field=0xb7fce198, tag=40092, szValuePtr=0xb7fcd2dc " ",
ByteCount=74) at /home/draal/src/php-src/ext/exif/exif.c:2702
2702 xp_field->tag = tag;
(gdb) p xp_field->size
$1 = 1515870810

@Draal
Copy link
Author

Draal commented Apr 7, 2013

On 64bit platform bug exists return (size_t)-1 and check < 0 (it doesn't reproduce only because struct has zero value)

dummy_encoding_converter (to=0x7ffff1f42e10, to_length=0x7ffff1f42e18, from=0x7ffff7fe5d90 " ", from_length=74, encoding_to=0x0, encoding_from=0x0)
at /var/src/php-src/Zend/zend_multibyte.c:50
50 return (size_t)-1;
(gdb) bt
#0 dummy_encoding_converter (to=0x7ffff1f42e10, to_length=0x7ffff1f42e18, from=0x7ffff7fe5d90 " ", from_length=74, encoding_to=0x0, encoding_from=0x0)
at /var/src/php-src/Zend/zend_multibyte.c:50
#1 0x00000000007e46d1 in zend_multibyte_encoding_converter (to=0x7ffff1f42e10, to_length=0x7ffff1f42e18, from=0x7ffff7fe5d90 " ", from_length=74,
encoding_to=0x0, encoding_from=0x0) at /var/src/php-src/Zend/zend_multibyte.c:150
#2 0x000000000058464b in exif_process_unicode (ImageInfo=0x7fffffffa200, xp_field=0x7ffff1f42e10, tag=40092, szValuePtr=0x7ffff7fe5d90 " ", ByteCount=74)
at /var/src/php-src/ext/exif/exif.c:2705
#3 0x0000000000585513 in exif_process_IFD_TAG (ImageInfo=0x7fffffffa200, dir_entry=0x7ffff7fe5d22 "\234\234", offset_base=0x7ffff7fe5ca0 "MM",
IFDlength=2948, displacement=30, section_index=3, ReadNextIFD=1, tag_table=0x9c6480) at /var/src/php-src/ext/exif/exif.c:2984
#4 0x0000000000585ba7 in exif_process_IFD_in_JPEG (ImageInfo=0x7fffffffa200, dir_start=0x7ffff7fe5ca8 "", offset_base=0x7ffff7fe5ca0 "MM", IFDlength=2948,
displacement=30, section_index=3) at /var/src/php-src/ext/exif/exif.c:3138
#5 0x0000000000585e63 in exif_process_TIFF_in_JPEG (ImageInfo=0x7fffffffa200, CharBuf=0x7ffff7fe5ca0 "MM", length=2948, displacement=30)
at /var/src/php-src/ext/exif/exif.c:3215
#6 0x0000000000585f5a in exif_process_APP1 (ImageInfo=0x7fffffffa200, CharBuf=0x7ffff7fe5c98 "\v\214Exif", length=2956, displacement=22)
at /var/src/php-src/ext/exif/exif.c:3240
#7 0x000000000058654b in exif_scan_JPEG_header (ImageInfo=0x7fffffffa200) at /var/src/php-src/ext/exif/exif.c:3385
#8 0x0000000000587652 in exif_scan_FILE_header (ImageInfo=0x7fffffffa200) at /var/src/php-src/ext/exif/exif.c:3767
#9 0x00000000005881b9 in exif_read_file (ImageInfo=0x7fffffffa200, FileName=0x7ffff7fe0c38 "/home/draal/exif_encoding_crash.jpg", read_thumbnail=0,
read_all=0) at /var/src/php-src/ext/exif/exif.c:3906
#10 0x00000000005883b6 in zif_exif_read_data (ht=1, return_value=0x7ffff7fe0bc0, return_value_ptr=0x0, this_ptr=0x0, return_value_used=1)
at /var/src/php-src/ext/exif/exif.c:3959
#11 0x00000000008079a4 in zend_do_fcall_common_helper_SPEC (execute_data=0x7ffff7fab0e8) at /var/src/php-src/Zend/zend_vm_execute.h:642
#12 0x000000000080eed4 in ZEND_DO_FCALL_SPEC_CONST_HANDLER (execute_data=0x7ffff7fab0e8) at /var/src/php-src/Zend/zend_vm_execute.h:2223
#13 0x00000000008061f4 in execute (op_array=0x7ffff7fe1b78) at /var/src/php-src/Zend/zend_vm_execute.h:410
#14 0x00000000007c776e in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /var/src/php-src/Zend/zend.c:1315
#15 0x000000000073b422 in php_execute_script (primary_file=0x7fffffffddb0) at /var/src/php-src/main/main.c:2492
#16 0x00000000009196ac in do_cli (argc=2, argv=0x7fffffffe168) at /var/src/php-src/sapi/cli/php_cli.c:988
#17 0x000000000091a5c1 in main (argc=2, argv=0x7fffffffe168) at /var/src/php-src/sapi/cli/php_cli.c:1364

@smalyshev
Copy link
Contributor

@Draal that's the thing, for me it doesn't call dummy_encoding_converter and doesn't return -1. Could you send me your compile options and php.ini to [email protected]?

@Draal
Copy link
Author

Draal commented Apr 8, 2013

@smalyshev
git status

On branch PHP-5.4.13

./buildconf --force
./configure --enable-exif --enable-debug
make

gcc -v
Using built-in specs.
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla --enable-bootstrap --enable-shared --enable-threads=posix --enable-checking=release --with-system-zlib --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-languages=c,c++,objc,obj-c++,java,fortran,ada --enable-java-awt=gtk --disable-dssi --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre --enable-libgcj-multifile --enable-java-maintainer-mode --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib --with-ppl --with-cloog --with-tune=generic --with-arch_32=i686 --build=x86_64-redhat-linux
Thread model: posix
gcc version 4.4.6 20120305 (Red Hat 4.4.6-4) (GCC)

@Draal
Copy link
Author

Draal commented Jun 5, 2013

Maybe is time to close?

@smalyshev
Copy link
Contributor

@Draal well, I'd be happy if we could figure out why you see it and I do not... maybe somebody else on the internals list to confirm it?

@Draal
Copy link
Author

Draal commented Jun 6, 2013

@smalyshev Did You build PHP for the 32bit platform and tried to reproduce the bug?

@smalyshev
Copy link
Contributor

@Draal yes, built it on 32-bit and it works fine on the file attached to this pull request.

@m6w6
Copy link
Contributor

m6w6 commented Oct 9, 2013

The code is definitely wrong and the fix seems ok, so, I'll merge soon as long as nobody's gonna beat me up.

php-pulls pushed a commit that referenced this pull request Oct 21, 2013
By:
	Draal
Conflicts:
	configure.in
	main/php_version.h
php-pulls pushed a commit that referenced this pull request Oct 21, 2013
* PHP-5.4:
  add tests for bug #62523
  Merged PR #293 (Exif crash on unknown encoding was fixed) By: 	Draal Conflicts: 	configure.in 	main/php_version.h
php-pulls pushed a commit that referenced this pull request Oct 21, 2013
* PHP-5.5:
  add tests for bug #62523
  Merged PR #293 (Exif crash on unknown encoding was fixed) By: 	Draal Conflicts: 	configure.in 	main/php_version.h
@php-pulls
Copy link

Comment on behalf of mike at php.net:

.

@php-pulls php-pulls closed this Oct 21, 2013
php-pulls pushed a commit that referenced this pull request Oct 22, 2013
* 'PHP-5.4' of git.php.net:php-src: (101 commits)
  exif NEWS
  add tests for bug #62523
  Merged PR #293 (Exif crash on unknown encoding was fixed) By: 	Draal Conflicts: 	configure.in 	main/php_version.h
  Just SKIP that test on travis
  fix memory leak on error (from Coverity scan)
  Fix coverity issue with -1 returned by findOffset not being handled by getPreferredTag
  5.4.21 release date
  When src->src is null this doesn't get initialized but it is still used, so the passed in *ze will point to unitialized memory. Hopefully src->src is never null, but just in case this initialization doesn't hurt.
  Fix typo
  Clean up this weird safe_emalloc() call
  Minor Coverity tweaks
  - Moved NULL check before dereferencing
  - Fixed possible NULL ptr dereference
  - Fixed possible uninitialized scalar variable usage (spotted by Coverity)
  Remove senseless check here
  - Fix extern declaration according to definition
  - Fix possible memory leak
  - Moved allocation to if block to make Coverity happy
  - Fixed possible memory leak
  Fix unitialized opened_path here - found by Coverity
  ...
php-pulls pushed a commit that referenced this pull request Oct 22, 2013
* 'PHP-5.5' of git.php.net:php-src: (178 commits)
  Fixed bug #65939 (Space before ";" breaks php.ini parsing). (brainstorm at nopcode dot org)
  exif NEWS
  add tests for bug #62523
  Merged PR #293 (Exif crash on unknown encoding was fixed) By: 	Draal Conflicts: 	configure.in 	main/php_version.h
  fix bug #65936 (dangling context pointer causes crash)
  remove TRAVIS check in test source
  Fixed compilation warning
  Just SKIP that test on travis
  Fixed issue #115 (path issue when using phar).
  fix memory leak on error (from Coverity scan)
  Fix coverity issue with -1 returned by findOffset not being handled by getPreferredTag
  5.4.21 release date
  fix argument type & remove warning
  fix const warnings in intl methods
  When src->src is null this doesn't get initialized but it is still used, so the passed in *ze will point to unitialized memory. Hopefully src->src is never null, but just in case this initialization doesn't hurt.
  Fix coverity issue with -1 returned by findOffset not being handled by getPreferredTag
  fix possibility of access to *storedType without initialization
  5.4.21 release date
  Fix typo
  These getpwnam('') tests are silly and not portable
  ...
php-pulls pushed a commit that referenced this pull request Oct 22, 2013
* 'master' of git.php.net:php-src: (270 commits)
  Fixed bug #65939 (Space before ";" breaks php.ini parsing). (brainstorm at nopcode dot org)
  exif NEWS
  add tests for bug #62523
  Merged PR #293 (Exif crash on unknown encoding was fixed) By: 	Draal Conflicts: 	configure.in 	main/php_version.h
  fix bug #65936 (dangling context pointer causes crash)
  remove TRAVIS check in test source
  Fixed compilation warning
  Just SKIP that test on travis
  Fixed issue #115 (path issue when using phar).
  fix memory leak on error (from Coverity scan)
  fix argument type & remove warning
  fix const warnings in intl methods
  Fix coverity issue with -1 returned by findOffset not being handled by getPreferredTag
  fix possibility of access to *storedType without initialization
  Fix coverity issue with -1 returned by findOffset not being handled by getPreferredTag
  5.4.21 release date
  fix argument type & remove warning
  fix const warnings in intl methods
  When src->src is null this doesn't get initialized but it is still used, so the passed in *ze will point to unitialized memory. Hopefully src->src is never null, but just in case this initialization doesn't hurt.
  Fix coverity issue with -1 returned by findOffset not being handled by getPreferredTag
  ...
php-pulls pushed a commit that referenced this pull request Oct 28, 2013
* master: (79 commits)
  ldap_escape() notes
  Increment version number, since this will be 5.5.6.
  Added Zend Debugger to the note about the load order (by trash4you at online dot de)
  Added a LICENSE file to make it easier for PECL binary distributions to conform with the license.
  Fix Coverity issue reporting wrong sizeof()
  Fixed bug #65939 (Space before ";" breaks php.ini parsing). (brainstorm at nopcode dot org)
  exif NEWS
  add tests for bug #62523
  Merged PR #293 (Exif crash on unknown encoding was fixed) By: 	Draal Conflicts: 	configure.in 	main/php_version.h
  fix bug #65936 (dangling context pointer causes crash)
  remove TRAVIS check in test source
  Fixed compilation warning
  Just SKIP that test on travis
  Fixed issue #115 (path issue when using phar).
  fix memory leak on error (from Coverity scan)
  fix argument type & remove warning
  fix const warnings in intl methods
  Fix coverity issue with -1 returned by findOffset not being handled by getPreferredTag
  fix possibility of access to *storedType without initialization
  Fix coverity issue with -1 returned by findOffset not being handled by getPreferredTag
  ...

Conflicts:
	Zend/zend_compile.c
	ext/intl/collator/collator_create.c
	ext/intl/locale/locale_methods.c
	ext/intl/msgformat/msgformat_format.c
	ext/intl/msgformat/msgformat_parse.c
rlerdorf added a commit that referenced this pull request Nov 8, 2013
* 'PHP-5.4' of git.php.net:php-src: (65 commits)
  Add a couple more test cases to parse_url() tests
  fix missing change from 'tcp_socket' to the more common 'server'
  fix many parallel test issues
  Cleanup temp test file
  Fixed Bug #66034 (Segmentation Fault when constructor of PDO statement throws an exception)
  Typo fix: umknown -> unknown
  Fix bug #66008
  5.4.23-dev
  Update NEWS
  Fixed Bug 64760 var_export() does not use full precision for floating-point numbers
  add bundled libzip LICENSE, as required by BSD License terms
  - Updated to version 2013.8 (2013h)
  remove "PHP 6" staff
  Fixed bug #65950 Field name truncation if the field name is bigger than 32 characters
  - Updated to version 2013.7 (2013g)
  Fix Coverity issue reporting wrong sizeof()
  exif NEWS
  add tests for bug #62523
  Merged PR #293 (Exif crash on unknown encoding was fixed) By: 	Draal Conflicts: 	configure.in 	main/php_version.h
  Just SKIP that test on travis
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants