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

Fix memory leak of function attribute hash table #8070

Closed
wants to merge 4 commits into from

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Feb 9, 2022

Found while implementing #7921:

==56540== 272 (56 direct, 216 indirect) bytes in 1 blocks are definitely lost in loss record 8 of 8
==56540== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==56540== by 0x6D2C62: __zend_malloc (zend_alloc.c:3055)
==56540== by 0x73E874: zend_add_attribute (zend_attributes.c:282)
==56540== by 0x65BE40: zend_add_parameter_attribute (zend_attributes.h:104)
==56540== by 0x65C760: zm_startup_password (password.c:452)
==56540== by 0x592AC7: zm_startup_basic (basic_functions.c:364)
==56540== by 0x71B18D: zend_startup_module_ex (zend_API.c:2202)
==56540== by 0x71B1EC: zend_startup_module_zval (zend_API.c:2217)
==56540== by 0x72CF48: zend_hash_apply (zend_hash.c:2011)
==56540== by 0x71B8F0: zend_startup_modules (zend_API.c:2328)
==56540== by 0x66CE1B: php_module_startup (main.c:2256)
==56540== by 0x887FC8: php_cli_startup (php_cli.c:409)

Copy link
Contributor

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Could you please add a regression test?

Zend/zend_opcode.c Outdated Show resolved Hide resolved
@cmb69
Copy link
Contributor

cmb69 commented Feb 10, 2022

Also, shouldn't this target PHP-8.1?

@TimWolla
Copy link
Member Author

Could you please add a regression test?

How would I write a test that checks for the absence of memory leaks?

@TimWolla
Copy link
Member Author

Also, shouldn't this target PHP-8.1?

Probably PHP 8.0, as that's where attributes were introduced. Shall I cherry-pick the commit onto 8.0, force push and then change the target branch in GH's interface?

@cmb69
Copy link
Contributor

cmb69 commented Feb 10, 2022

How would I write a test that checks for the absence of memory leaks?

Just write a test that triggers the code path; we run nightly builds with MSan/ASan enabled, which should catch this.

Shall I cherry-pick the commit onto 8.0, force push and then change the target branch in GH's interface?

I usually rebase the local branch onto the new target branch (PHP-8.0, right), force-push, and then change the target branch in the GH UI. Otherwise, some CI get's confused (AppVeyor, and maybe some others).

@TimWolla
Copy link
Member Author

Just write a test that triggers the code path; we run nightly builds with MSan/ASan enabled, which should catch this.

This is impossible to trigger as of right now, as there are no native functions with attributes (neither function-level, nor parameter level attributes). As mentioned in my initial message this was found during development of #7921 (which is currently in voting).

@cmb69
Copy link
Contributor

cmb69 commented Feb 10, 2022

Ah, right. Then it is okay without a test. :)

@TimWolla
Copy link
Member Author

Ah, right. Then it is okay without a test. :)

Then just the rebase? Or shall this remain on master? I can't really test the changes with 8.0 for the same reason as I can't write an actual test.

@cmb69
Copy link
Contributor

cmb69 commented Feb 10, 2022

Good point! I'm not sure whether there are any (PECL) extensions which use attributes; that memory leak might be relevant to them. Maybe @beberlei knows?

@beberlei
Copy link
Contributor

@TimWolla @cmb69 not that i know of the top of my head.

@TysonAndre
Copy link
Contributor

This is impossible to trigger as of right now, as there are no native functions with attributes (neither function-level, nor parameter level attributes). As mentioned in my initial message this was found during development of 7921 (which is currently in voting).

https://github.com/php/php-src/blob/master/ext/zend_test/test.stub.php The zend_test extension is used for things that are only used in php's test cases, not in public functionality. I'm fine with not adding to zend_test since it seems like the rfc is likely to pass and we can test with that instead, and the fix is simple

@TysonAndre
Copy link
Contributor

looks correct to me

@TimWolla
Copy link
Member Author

@TysonAndre Thank you for the review and the hint regarding zend_test.

@cmb69 Is this PR looking good to you, or would you like me to make changes? Which target branch should this use? I can integrate zend_test, if you think that would be good.

@cmb69
Copy link
Contributor

cmb69 commented Feb 14, 2022

@TimWolla, yes that looks good to me. Maybe it's best to target "master" for now; we can still back-port if actually relevant. Do you want to add a test right away?

@TimWolla
Copy link
Member Author

@cmb69 I've rebased onto the current master, added a test and updated the commit message with the valgrind output for the function in zend_test. PTAL!

@TimWolla
Copy link
Member Author

Okay, so Cirrus is happy, but Travis is unhappy about this. Probably good that I added a test. However I'm afraid that fixing this double free is above my pay grade. I can't do more than NULLing out that value.

Zend/zend_opcode.c Outdated Show resolved Hide resolved
Zend/zend_opcode.c Outdated Show resolved Hide resolved
@TimWolla
Copy link
Member Author

Okay, I was hoping that using zend_hash_release() would fix it, but unfortunately it doesn't. It still works on my local computer (and is arguably cleaner than the explicit free()), but Travis still is unhappy.

I'm at my wits end here and frankly I can't spend any more time on this, because this is pure guesswork on my end. Someone more experienced than me will need to take this across the finish line.

@TimWolla
Copy link
Member Author

Ah, this is only reproducible with ZTS:

==631692== Invalid read of size 4
==631692==    at 0x73E026: zend_hash_release (zend_hash.h:357)
==631692==    by 0x73E72A: zend_function_dtor (zend_opcode.c:157)
==631692==    by 0x7715D0: zend_hash_destroy (zend_hash.c:1689)
==631692==    by 0x7532B4: compiler_globals_dtor (zend.c:724)
==631692==    by 0x691536: ts_free_id (TSRM.c:541)
==631692==    by 0x753F92: zend_shutdown (zend.c:1106)
==631692==    by 0x69807C: php_module_shutdown (main.c:2410)
==631692==    by 0x8DDB2A: main (php_cli.c:1382)
==631692==  Address 0x4e64754 is 4 bytes inside a block of size 56 free'd
==631692==    at 0x483CA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==631692==    by 0x73E075: zend_hash_release (zend_hash.h:360)
==631692==    by 0x73E72A: zend_function_dtor (zend_opcode.c:157)
==631692==    by 0x7715D0: zend_hash_destroy (zend_hash.c:1689)
==631692==    by 0x753EC5: zend_shutdown (zend.c:1082)
==631692==    by 0x69807C: php_module_shutdown (main.c:2410)
==631692==    by 0x8DDB2A: main (php_cli.c:1382)
==631692==  Block was alloc'd at
==631692==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==631692==    by 0x7116A7: __zend_malloc (zend_alloc.c:3068)
==631692==    by 0x7840EC: zend_add_attribute (zend_attributes.c:226)
==631692==    by 0x6888AD: zend_add_parameter_attribute (zend_attributes.h:102)
==631692==    by 0x68C10B: zm_startup_zend_test (test.c:478)
==631692==    by 0x76023F: zend_startup_module_ex (zend_API.c:2202)
==631692==    by 0x7602B3: zend_startup_module_zval (zend_API.c:2217)
==631692==    by 0x7728E3: zend_hash_apply (zend_hash.c:2011)
==631692==    by 0x7609DB: zend_startup_modules (zend_API.c:2328)
==631692==    by 0x697BCE: php_module_startup (main.c:2256)
==631692==    by 0x8DB58A: php_cli_startup (php_cli.c:409)
==631692==    by 0x8DD99E: main (php_cli.c:1334)

@TysonAndre
Copy link
Contributor

I'm at my wits end here and frankly I can't spend any more time on this, because this is pure guesswork on my end. Someone more experienced than me will need to take this across the finish line.

I'm only moderately familiar with this code and have gotten stuck and frustrated a lot, especially with opcache. Someone more familiar than me will probably have to take a look. Leaving more context for people looking at this later:

https://github.com/php/php-src/tree/master/ext/opcache/jit#running-tests-of-the-jit running locally with --repeat 2 may help, it's apparently what travis would use. I don't know why it'd be opcache or the jit, but being aware of the different options used in CI and trying them may help (requires building php with opcache support, see the rest of the README)

  • I think opcache would only matter for internal methods with parameter attributes that get subclassed in userland (e.g. ext/pdo/), but I'm uncertain of this and only moderately familiar with opcache. I'm not even sure if attribute support for internals is correct for opcache.

Some of these may help, in combination with -m for valgrind:

  • --repeat 2
  • -d zend_extension=modules/opcache.so -d opcache.enable -d opcache.enable_cli=1 -d opcache.protect_memory=1 may help.

.travis.yml: - if [[ "$ARM64" == 1 ]]; then ./travis/test.sh -d opcache.jit_buffer_size=16M -d opcache.jit=tracing --repeat 2; fi - the ARM64 travis builds repeat tests twice, so that may explain double frees in one case. But the other travis job is failing.

# Run PHPs run-tests.php
script:
    - ./travis/test.sh -d opcache.jit_buffer_size=16M -d opcache.jit=tracing
    - if [[ "$ARM64" == 1 ]]; then ./travis/test.sh -d opcache.jit_buffer_size=16M -d opcache.jit=function; fi
    - if [[ "$ARM64" == 1 ]]; then ./travis/test.sh -d opcache.jit_buffer_size=16M -d opcache.jit=tracing --repeat 2; fi
    - sapi/cli/php -d extension_dir=`pwd`/modules -r 'dl("zend_test");'

@TimWolla
Copy link
Member Author

TimWolla commented Feb 14, 2022

@TysonAndre Thank you for taking the time to assist me, I appreciate it and the information are helpful.

However I believe I figured it out: There was a second bug lurking in the code, there was a missing call to CG_ADDREF() for the hash table. I'm currently waiting for Travis to report the results, will then rebase the branch and then request another review from you and cmb in GitHub's interface.

@TysonAndre
Copy link
Contributor

Additional tests may help detect if this is properly finished, e.g.

  1. Internal class with a method with a parameter attribute
  2. Userland subclass of 1.
  3. Internal subclass of 1.

It seems like this should use the GC_TRY_ADDREF instead of GC_ADDREF macro because attributes can be an immutable array after adding it to opcache's shared memory (for methods, maybe copying from opcache?), not GC_ADDREF.

static HashTable *zend_persist_attributes(HashTable *attributes)
{
	// ...

	HashTable *ptr = zend_shared_memdup_put_free(attributes, sizeof(HashTable));
	GC_SET_REFCOUNT(ptr, 2);
	GC_TYPE_INFO(ptr) = GC_ARRAY | ((IS_ARRAY_IMMUTABLE|GC_NOT_COLLECTABLE) << GC_FLAGS_SHIFT);
static zend_always_inline uint32_t zend_gc_addref(zend_refcounted_h *p) {
	ZEND_RC_MOD_CHECK(p);
	return ++(p->refcount);
}

static zend_always_inline void zend_gc_try_addref(zend_refcounted_h *p) {
	if (!(p->u.type_info & GC_IMMUTABLE)) {
		ZEND_RC_MOD_CHECK(p);
		++p->refcount;
	}
}

Zend/zend_compile.c Outdated Show resolved Hide resolved
@TimWolla
Copy link
Member Author

Additional tests may help detect if this is properly finished, e.g.

@TysonAndre You are correct: The code was not yet correct for class inheritance. I've now (hopefully) fixed those as well.

@TimWolla
Copy link
Member Author

Okay:

  • FreeBSD NTS is green.
  • AppVeyor is green.
  • Travis s390x is green.
  • Travis ARM is red (but it's also red in master, thus likely not my fault).
  • Azure is red (but that's currently broken).

So I believe this is ready to go 😃

@TimWolla TimWolla requested a review from cmb69 February 14, 2022 22:24
@TysonAndre
Copy link
Contributor

TysonAndre commented Feb 14, 2022

Tests look right and my previous review comments were addressed. ext/opcache/tests/jit/assign_dim_op_007.phpt for travis has nothing to do with attributes, and it's a new test.

I don't see anything wrong with it but would (personally) like someone more familiar with opcache to take a look at this before merging it.

@TimWolla
Copy link
Member Author

Rebased onto the latest master to trigger the new GitHub Actions builds. PTAL 😃

Zend/zend_compile.c Outdated Show resolved Hide resolved
Zend/zend.c Outdated Show resolved Hide resolved
ext/zend_test/test.c Outdated Show resolved Hide resolved
Zend/zend.c Show resolved Hide resolved
bwoebi and others added 4 commits March 7, 2022 21:21
    ==109253== 280 (56 direct, 224 indirect) bytes in 1 blocks are definitely lost in loss record 4 of 4
    ==109253==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==109253==    by 0x6D9FA2: __zend_malloc (zend_alloc.c:3068)
    ==109253==    by 0x745138: zend_add_attribute (zend_attributes.c:226)
    ==109253==    by 0x6680D1: zend_add_parameter_attribute (zend_attributes.h:102)
    ==109253==    by 0x66B787: zm_startup_zend_test (test.c:478)
    ==109253==    by 0x7224CD: zend_startup_module_ex (zend_API.c:2202)
    ==109253==    by 0x72252C: zend_startup_module_zval (zend_API.c:2217)
    ==109253==    by 0x734288: zend_hash_apply (zend_hash.c:2011)
    ==109253==    by 0x722C30: zend_startup_modules (zend_API.c:2328)
    ==109253==    by 0x67409B: php_module_startup (main.c:2256)
    ==109253==    by 0x88EDDE: php_cli_startup (php_cli.c:409)
    ==109253==    by 0x890F61: main (php_cli.c:1334)
These tests verify the correct workings of the previous fixes:

- Parameter attributes for native functions should not leak memory.
- Parameter attributes for native functions should behave as expected.
@TimWolla
Copy link
Member Author

TimWolla commented Mar 7, 2022

@bwoebi Thanks for your assistance. As the CI was green I've rebased all the trial and error commits into a clean commit history.

bwoebi pushed a commit that referenced this pull request Mar 7, 2022
    ==109253== 280 (56 direct, 224 indirect) bytes in 1 blocks are definitely lost in loss record 4 of 4
    ==109253==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
    ==109253==    by 0x6D9FA2: __zend_malloc (zend_alloc.c:3068)
    ==109253==    by 0x745138: zend_add_attribute (zend_attributes.c:226)
    ==109253==    by 0x6680D1: zend_add_parameter_attribute (zend_attributes.h:102)
    ==109253==    by 0x66B787: zm_startup_zend_test (test.c:478)
    ==109253==    by 0x7224CD: zend_startup_module_ex (zend_API.c:2202)
    ==109253==    by 0x72252C: zend_startup_module_zval (zend_API.c:2217)
    ==109253==    by 0x734288: zend_hash_apply (zend_hash.c:2011)
    ==109253==    by 0x722C30: zend_startup_modules (zend_API.c:2328)
    ==109253==    by 0x67409B: php_module_startup (main.c:2256)
    ==109253==    by 0x88EDDE: php_cli_startup (php_cli.c:409)
    ==109253==    by 0x890F61: main (php_cli.c:1334)
@bwoebi
Copy link
Member

bwoebi commented Mar 7, 2022

Merged as 0d7e10c on PHP 8.0. Applying the tests separately on PHP 8.1, because gen_stubs does not yet support attributes on the version present on PHP 8.0: applied as 070012d.

@bwoebi bwoebi closed this Mar 7, 2022
@TimWolla
Copy link
Member Author

TimWolla commented Mar 7, 2022

because gen_stubs does not yet support attributes on the version present on PHP 8.0

Whoops. I wanted to remove the attributes from the stub file anyway, because they don't do anything. Apparently I missed zend_test_parameter_with_attribute while doing so.

see also: #7921 (comment)

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.

None yet

7 participants