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 potential heap corruption due to alignment mismatch #9724

Closed
wants to merge 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Oct 11, 2022

The fix for bug 63327 changed the extra size of mysqlnd allocations from sizeof(size_t) to the properly aligned values; however, the allocation in _mysqlnd_pestrdup() has apparently been overlooked, which (currently) causes detectable heap corruption when running mysqli_get_client_stats.phpt on 32bit Windows versions.


There may be further issues; that should be checked. Apparently, there are no other issues in this regard.

The fix for bug 63327[1] changed the extra size of mysqlnd allocations
from `sizeof(size_t)` to the properly aligned values; however, the
allocation in `_mysqlnd_pestrdup()` has apparently been overlooked,
which (currently) causes detectable heap corruption when running
mysqli_get_client_stats.phpt on 32bit Windows versions.

[1] <php@338a47b>
@cmb69
Copy link
Member Author

cmb69 commented Oct 11, 2022

I've just noticed

#define MYSQLND_DEBUG_MEMORY 1

Has this perhaps accidentially committed with 37418de? @kamil-tekiela, thoughts?

@kamil-tekiela
Copy link
Member

I don't know. It wasn't an accident as far as I know. This is a user-facing functionality that can be enabled via INI setting collect_memory_statistics. Why would anyone ever use it is beyond me. This looks like some attempt by the developer to trace their code, but IMHO this should have all been removed before shipping the extension. The whole mysqlnd statistics should have never been shipped. But what do I know...

@cmb69
Copy link
Member Author

cmb69 commented Oct 12, 2022

Thanks, @kamil-tekiela! That explains the current behavior, but I agree that this is doubtful, and we should re-consider.

@cmb69 cmb69 closed this in 7e14d24 Oct 13, 2022
@cmb69 cmb69 deleted the cmb/mysqlnd-misalign branch October 13, 2022 09:50
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.

2 participants