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

ext/pdo: Fix memory leak if GC needs to free PDO Statement #17539

Closed

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Jan 21, 2025

No description provided.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

I tested with PDO::FETCH_LAZY too to check if the lazy object also needs to be added, but it seems that object is read only, so it seems we're fine.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Actually, your firebird test fails with a leak...

@Girgias
Copy link
Member Author

Girgias commented Jan 22, 2025

I think it's the "usual" Firebird leak, so I need to add the boiler plate for that

Girgias and others added 2 commits January 24, 2025 19:37
If both the driver object and statement end up in the GC buffer and are
freed by the GC, then the destruction order is not deterministic and it
is possible that the driver object is freed before the statement. In
that case, accessing S->H will cause a UAF. As the resources are already
released we simply skip the destruction if the driver object is already
destroyed.
@Girgias Girgias force-pushed the pdo-fix-memory-leak-cyclic-pdo-stmt branch from 4eaf084 to 68fcd47 Compare January 24, 2025 19:37
@Girgias Girgias closed this in e6d917e Jan 24, 2025
@Girgias Girgias deleted the pdo-fix-memory-leak-cyclic-pdo-stmt branch January 24, 2025 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants