Skip to content

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

Closed
andrewnester wants to merge 1 commit intophp:PHP-7.0from
andrewnester:73973-debug-zval-dump
Closed

Fixed #73973 - debug_zval_dump() assertion error for resource consts with --enable-debug#2332
andrewnester wants to merge 1 commit intophp:PHP-7.0from
andrewnester:73973-debug-zval-dump

Conversation

@andrewnester
Copy link
Contributor

@andrewnester andrewnester 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

php-src/Zend/zend_ast.c

Line 481 in c8aa6f3

if (!ast) {

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

@krakjoe krakjoe added the Bug label Jan 24, 2017
@krakjoe krakjoe self-assigned this Jan 24, 2017
@krakjoe krakjoe requested a review from nikic January 24, 2017 15:16
@krakjoe
Copy link
Member

krakjoe commented Jan 24, 2017 •
edited
Loading

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

@nikic
Copy link
Member

nikic commented Jan 24, 2017 •
edited
Loading

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
Copy link
Member

krakjoe 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
Copy link
Contributor Author

andrewnester commented Jan 24, 2017

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

@andrewnester
Copy link
Contributor Author

andrewnester commented Jan 24, 2017 •
edited
Loading

@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
Copy link
Contributor Author

andrewnester 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
Copy link
Member

nikic 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
Copy link
Contributor Author

andrewnester 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
Copy link
Member

krakjoe commented Jan 25, 2017

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

@andrewnester
Copy link
Contributor Author

andrewnester 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
Copy link
Contributor Author

andrewnester 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 73973-debug-zval-dump branch from 339815c to d8b6cc3 Compare January 25, 2017 13:51
@andrewnester
Copy link
Contributor Author

andrewnester 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
Copy link
Member

krakjoe 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
Copy link
Member

krakjoe commented Jan 25, 2017

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

@andrewnester
Copy link
Contributor Author

andrewnester 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
Copy link
Member

krakjoe 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
Copy link
Member

krakjoe commented Jan 26, 2017 •
edited
Loading

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

Reviewers

@nikic nikic Awaiting requested review from nikic

Assignees

@krakjoe krakjoe

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andrewnester @krakjoe @nikic