Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
andrewnester wants to merge 1 commit into from
Closed

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

andrewnester wants to merge 1 commit into from
+13 −12

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