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

Fixed #73973 - debug_zval_dump() assertion error for resource consts with --enable-debug #2332

Closed
wants to merge 1 commit into from

Conversation

@andrewnester
Copy link
Contributor

commented Jan 24, 2017

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

I reverted changes in zend_builtin_functions.c done here - 6815c08

Proper resource handling is done here 896814e and here ac58d21

Segmentation fault mentioned in https://bugs.php.net/bug.php?id=70398 will not occur because of

if (!ast) {

Corresponding to https://bugs.php.net/bug.php?id=70398 tests are passing successfully.

@krakjoe krakjoe added the Bugfix label Jan 24, 2017

@krakjoe krakjoe self-assigned this Jan 24, 2017

@krakjoe krakjoe requested a review from nikic Jan 24, 2017

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

This looks okay to me (and travis, but that's release mode) ... just waiting for some input from @nikic

@nikic

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

The two cited commits aren't related to this problem. As such, this simply reverts the original fix, such that bug73098.phpt valgrinds again. First error:

==19495== Invalid read of size 4
==19495==    at 0xC10EE0: i_zval_ptr_dtor (zend_variables.h:48)
==19495==    by 0xC157F0: zend_array_destroy (zend_hash.c:1305)
==19495==    by 0xBF7804: _zval_dtor_func (zend_variables.c:43)
==19495==    by 0xBDBD69: i_zval_ptr_dtor (zend_variables.h:49)
==19495==    by 0xBDF48A: _zval_ptr_dtor (zend_execute_API.c:550)
==19495==    by 0xBD9BEB: free_zend_constant (zend_constants.c:36)
==19495==    by 0xC149F7: _zend_hash_del_el_ex (zend_hash.c:997)
==19495==    by 0xC14AD7: _zend_hash_del_el (zend_hash.c:1020)
==19495==    by 0xC16754: zend_hash_reverse_apply (zend_hash.c:1602)
==19495==    by 0xBDA36B: clean_non_persistent_constants (zend_constants.c:161)
==19495==    by 0xBDEA1C: shutdown_executor (zend_execute_API.c:380)
==19495==    by 0xBFB603: zend_deactivate (zend.c:1066)
==19495==  Address 0xfa8bd80 is 0 bytes inside a block of size 24 free'd
==19495==    at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19495==    by 0xBB9016: _efree (zend_alloc.c:2428)
==19495==    by 0xC19BB6: list_entry_destructor (zend_list.c:189)
==19495==    by 0xC149F7: _zend_hash_del_el_ex (zend_hash.c:997)
==19495==    by 0xC1533B: zend_hash_index_del (zend_hash.c:1198)
==19495==    by 0xC1962F: zend_list_delete (zend_list.c:50)
==19495==    by 0xB5A024: _php_stream_free (streams.c:449)
==19495==    by 0xB59F65: _php_stream_free (streams.c:410)
==19495==    by 0xB5CEC1: stream_resource_regular_dtor (streams.c:1619)
==19495==    by 0xC19734: zend_resource_dtor (zend_list.c:76)
==19495==    by 0xC19D53: zend_close_rsrc (zend_list.c:230)
==19495==    by 0xC166FA: zend_hash_reverse_apply (zend_hash.c:1598)
==19495==  Block was alloc'd at
==19495==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==19495==    by 0xBBA0D3: __zend_malloc (zend_alloc.c:2820)
==19495==    by 0xBB8E9B: _emalloc (zend_alloc.c:2413)
==19495==    by 0xC194F5: zend_list_insert (zend_list.c:43)
==19495==    by 0xC197CA: zend_register_resource (zend_list.c:98)
==19495==    by 0xB59CC8: _php_stream_alloc (streams.c:310)
==19495==    by 0xB612C1: _php_stream_temp_create_ex (memory.c:566)
==19495==    by 0xB61387: _php_stream_temp_create (memory.c:578)
==19495==    by 0xA11AA3: php_stream_url_wrap_php (php_fopen_wrapper.c:211)
==19495==    by 0xB5E8EA: _php_stream_open_wrapper_ex (streams.c:2055)
==19495==    by 0x98DFF8: php_if_fopen (file.c:889)
==19495==    by 0x836196: phar_fopen (func_interceptors.c:427)
@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

Oh, I didn't valgrind it ...

Looks like we're back to square one then ... have you had any ideas yet @nikic ?

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

@nikic thanks! I will investigate further and re-work this PR then

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2017

@nikic @krakjoe I just have 1 concern - as I understood previously (before the fix introduced) test bug70398.phpt has been failing with segfault. Now, without that fix - no segfault, but valgrind fails. Might be problem in some other place?

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2017

@nikic @krakjoe I was trying to valgrind test bug70398.phpt on the build from my branch on both OS X and Ubuntu, but there was no fails, @nikic could you please provide more details how you got this failure? Thanks

@nikic

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

Did you run valgrind with USE_ZEND_ALLOC=0? You can also invoke the test runner with valgrind using sapi/cli/php run-tests.php -P -m Zend/tests/bug70398.phpt.

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2017

@nikic yeah, sure, I set USE_ZEND_ALLOC=0. Tried to run it through run-tests.php and valgrind - both ways no segfaults

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

You're not looking for faults, you're looking for memory errors reported by valgrind.

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2017

@krakjoe yeah, sure, I know :) I mean I see no reports indicating possible segfault or in general any failure similar to what @nikic previously sent to us.

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2017

@krakjoe @nikic finally reproduced it, there were some configuration issues, thanks for the help! I'm continuing investigating this issue.

@andrewnester andrewnester force-pushed the andrewnester:73973-debug-zval-dump branch from 339815c to d8b6cc3 Jan 25, 2017

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2017

Hi again @nikic @krakjoe !
I've updated my PR. As a fix I changed order of cleaning constant and resources (moved cleaning constants before cleaning resource list). Cleaning resource list has been introduced here
91ed685#diff-856f8846e4ed94f923a0978e75c75b17

and here is the old note I found that constants should be destroyed before resource list a35c61a#diff-856f8846e4ed94f923a0978e75c75b17R224

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

Great stuff, I'll have time to review later on today ... and @nikic will tell us if something is obviously wrong, I hope :)

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

The bug reported in http://bugs.php.net/73973 remains.

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2017

Hi @krakjoe ! Not sure if I properly understood you but let me explain. Before my fix I've added test bug73973.phpt which has been failing with exactly same error described in http://bugs.php.net/73973

After my fix this test passed successfully (locally on Ubuntu and OS X and on Travis). Also I've checked this test for memory errors with valgrind - everything looks fine.

Could you please give me more details, I will verify it. Thanks!

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 26, 2017

My mistake, I rebuilt this morning and there is no error ... must have been using wrong php executable or something.

LGTM.

@krakjoe

This comment has been minimized.

Copy link
Member

commented Jan 26, 2017

Merged f65ae82

Thanks.

@krakjoe krakjoe closed this Jan 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.