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

Memory leak when calling Randomizer::__construct() on an existing object #9089

Closed
TimWolla opened this issue Jul 21, 2022 · 4 comments · Fixed by #9091
Closed

Memory leak when calling Randomizer::__construct() on an existing object #9089

TimWolla opened this issue Jul 21, 2022 · 4 comments · Fixed by #9091

Comments

@TimWolla
Copy link
Member

Description

The following code:

<?php

$r = new Random\Randomizer();
$r->__construct();

Resulted in this output:

$ valgrind --leak-check=full sapi/cli/php test.php
==214630== Memcheck, a memory error detector
==214630== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==214630== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==214630== Command: sapi/cli/php test.php
==214630== 

Fatal error: Uncaught Error: Cannot modify readonly property Random\Randomizer::$engine in php-src/test.php:4
Stack trace:
#0 php-src/test.php(4): Random\Randomizer->__construct()
#1 {main}
  thrown in php-src/test.php on line 4
==214630== 
==214630== HEAP SUMMARY:
==214630==     in use at exit: 1,150 bytes in 21 blocks
==214630==   total heap usage: 17,921 allocs, 17,900 frees, 3,357,121 bytes allocated
==214630== 
==214630== 56 bytes in 1 blocks are definitely lost in loss record 19 of 21
==214630==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==214630==    by 0x76B62E: __zend_malloc (zend_alloc.c:3088)
==214630==    by 0x769E7F: _malloc_custom (zend_alloc.c:2439)
==214630==    by 0x76A040: _emalloc (zend_alloc.c:2558)
==214630==    by 0x52BA9A: zend_object_alloc (zend_objects_API.h:92)
==214630==    by 0x52C892: php_random_engine_common_init (random.c:271)
==214630==    by 0x52C5BC: php_random_engine_secure_new (random.c:216)
==214630==    by 0x5336AC: zim_Random_Randomizer___construct (randomizer.c:75)
==214630==    by 0x7F5365: ZEND_DO_FCALL_SPEC_RETVAL_UNUSED_HANDLER (zend_vm_execute.h:1776)
==214630==    by 0x8725EA: execute_ex (zend_vm_execute.h:55926)
==214630==    by 0x877DF4: zend_execute (zend_vm_execute.h:60253)
==214630==    by 0x7B1014: zend_execute_scripts (zend.c:1770)
==214630== 
==214630== LEAK SUMMARY:
==214630==    definitely lost: 56 bytes in 1 blocks
==214630==    indirectly lost: 0 bytes in 0 blocks
==214630==      possibly lost: 0 bytes in 0 blocks
==214630==    still reachable: 1,094 bytes in 20 blocks
==214630==         suppressed: 0 bytes in 0 blocks
==214630== Reachable blocks (those to which a pointer was found) are not shown.
==214630== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==214630== 
==214630== For lists of detected and suppressed errors, rerun with: -s
==214630== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

But I expected this output instead: No leak.

PHP Version

Current git master

Operating System

No response

@TimWolla
Copy link
Member Author

I suspected that the call to GC_DELREF

GC_DELREF(engine_object);

needs to be moved after zend_update_property(), but I was unsuccessful fixing the leak with that.

@TimWolla
Copy link
Member Author

Likely unrelated, but it also feels a little unclean to directly call ->create_object()

engine_object = random_ce_Random_Engine_Secure->create_object(random_ce_Random_Engine_Secure);

instead of going through something like object_init_ex() (is this the right function for this case?).

@zeriyoshi
Copy link
Contributor

This was completely unintended.

I have a feeling that other extensions have had similar problems in the past, so I will look into an appropriate response tomorrow.

@TimWolla
Copy link
Member Author

so I will look into an appropriate response tomorrow.

Don't stress yourself too much. This one is a fairly obscure issue that is not likely to happen in the "real world". So if it takes a week or two (or even a month), then that's also okay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants