Skip to content

Conversation

@el7cosmos
Copy link
Contributor

@el7cosmos el7cosmos commented Sep 20, 2025

Description

Embed::run_script missing call zend_destroy_file_handle() and may cause a memory leak.

Example leak with valgrind:

==648== 112 bytes in 2 blocks are definitely lost in loss record 34 of 87
==648==    at 0x488545C: malloc (vg_replace_malloc.c:446)
==648==    by 0x513DAAB: __zend_malloc (zend_alloc.c:3294)
==648==    by 0x513C1EF: _emalloc (zend_alloc.c:2740)
==648==    by 0x532B55F: zend_string_alloc (zend_string.h:176)
==648==    by 0x532B5D3: zend_string_init (zend_string.h:198)
==648==    by 0x532BAEB: zend_stream_init_filename (zend_stream.c:73)
==648==    by 0x28F73B: ext_php_rs::embed::Embed::run_script (mod.rs:105)
==648==    by 0x2E2383: pasir::service::php::execute_php::{{closure}}::{{closure}} (php.rs:182)
==648==    by 0x214703: ext_php_rs::zend::try_catch::panic_wrapper::{{closure}} (try_catch.rs:17)
==648==    by 0x25A207: std::panicking::catch_unwind::do_call (panicking.rs:590)
==648==    by 0x1EEF07: __rust_try (in /app/target/debug/pasir)
==648==    by 0x1EDCF3: std::panic::catch_unwind (panicking.rs:553)

Checklist

Check the boxes that apply (put an x in the brackets, like [x]). You can also check boxes after the PR is created.

❤️ Thank you for your contribution!

@coveralls
Copy link

coveralls commented Sep 20, 2025

Pull Request Test Coverage Report for Build 18141211519

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 27.728%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/embed/mod.rs 0 1 0.0%
Totals Coverage Status
Change from base Build 18140982423: -0.007%
Covered Lines: 1156
Relevant Lines: 4169

💛 - Coveralls

@el7cosmos el7cosmos force-pushed the embed-run-script-leak branch from 444fce1 to 3279b17 Compare September 20, 2025 06:50
@ptondereau
Copy link
Member

Good catch!
Could you also fix clippy errors? If not, we'll do it in another PR
Thank you

@el7cosmos
Copy link
Contributor Author

I can try to fix Clippy errors

@Xenira
Copy link
Member

Xenira commented Sep 30, 2025

@el7cosmos sorry for the long delay. Fixed clippy, embed and bindings in master.

@Xenira Xenira force-pushed the embed-run-script-leak branch from 192c87b to 9e40535 Compare September 30, 2025 19:28
@Xenira Xenira merged commit 113ef33 into extphprs:master Sep 30, 2025
55 checks passed
@davidcole1340 davidcole1340 mentioned this pull request Sep 30, 2025
@el7cosmos el7cosmos deleted the embed-run-script-leak branch October 30, 2025 07:03
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.

4 participants