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

There is a memory leak defect at line 2214 in /php-src/main/streams/streams.c. #15026

Closed
LuMingYinDetect opened this issue Jul 19, 2024 · 6 comments

Comments

@LuMingYinDetect
Copy link

Description

A pointer variable named resolved_path is defined at line 2186 in the file /php-src/main/streams/streams.c. This variable is allocated a block of dynamic memory via the function php_resolve_path at line 2205. When the if statement at line 2213 evaluates to true, the function returns at line 2214. However, unlike at line 2224 where the dynamically allocated memory pointed to by resolved_path is freed before returning, in this case, it leads to a memory leak defect, as illustrated below:
https://github.com/LuMingYinDetect/php_defects/blob/main/php_3.png

PHP Version

PHP 8.4.0

Operating System

Ubuntu 22.04

@nielsdos
Copy link
Member

Hmm, isn't resolved_path NULL when EG(exception) is set? Otherwise how could we have an exception and a path at the same time?

@LuMingYinDetect
Copy link
Author

Hmm, isn't resolved_path NULL when EG(exception) is set? Otherwise how could we have an exception and a path at the same time?

Thank you for your prompt reply! I didn't notice this issue when verifying the defect report; it should be a false alarm.

@nielsdos
Copy link
Member

No problem, thanks for the other reports btw.
I have opened a PR to add an assertion that the path is NULL in case an exception was thrown. This should make the developer intent clear and prevent further reports of this.

@LuMingYinDetect
Copy link
Author

No problem, thanks for the other reports btw. I have opened a PR to add an assertion that the path is NULL in case an exception was thrown. This should make the developer intent clear and prevent further reports of this.

I am happy to contribute to PHP. In fact, receiving prompt responses from developers helps us improve our static analysis tool more effectively, ensuring better outcomes.Currently, our static analysis tool still has some bugs. Thank you very much for your feedback on false positives!

@nielsdos
Copy link
Member

@LuMingYinDetect Turns out this is in fact a true positive, see #15908. I'll make sure to credit you in the test file.

nielsdos added a commit to nielsdos/php-src that referenced this issue Sep 16, 2024
This was first reported as a leak in phpGH-15026, but was mistakingly
believed to be a false positive. Then an assertion was added and it got
triggered in phpGH-15908. This fixes the leak. Upon merging into master the
assertion should be removed as well.
@LuMingYinDetect
Copy link
Author

事实证明这实际上是一个真正的阳性,请参阅 #15908。我一定会在测试文件中注明你的功劳。

Thank you for your patient response! We greatly appreciate your affirmation of our work!

nielsdos added a commit that referenced this issue Sep 22, 2024
This was first reported as a leak in GH-15026, but was mistakingly
believed to be a false positive. Then an assertion was added and it got
triggered in GH-15908. This fixes the leak. Upon merging into master the
assertion should be removed as well.

Closes GH-15924.
nielsdos added a commit that referenced this issue Sep 22, 2024
* PHP-8.2:
  Fix GH-15908 and GH-15026: leak / assertion failure in streams.c
nielsdos added a commit that referenced this issue Sep 22, 2024
* PHP-8.3:
  Fix GH-15908 and GH-15026: leak / assertion failure in streams.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants