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

Initial ZendMM conversion of glob #17433

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NattyNarwhal
Copy link
Member

Removes the vendored OpenBSD reallocarray and changes libc memory management to ZendMM.

Not sure if non-persistent is the right call, I think glob users use it then call globfree ASAP within a request, but it might be preferable to use persistent allocations.

@NattyNarwhal NattyNarwhal marked this pull request as ready for review January 10, 2025 18:00
win32/glob.c Outdated Show resolved Hide resolved
win32/glob.c Outdated Show resolved Hide resolved
win32/glob.c Outdated Show resolved Hide resolved
win32/glob.c Outdated Show resolved Hide resolved
@bukka
Copy link
Member

bukka commented Jan 19, 2025

About the persistence, it should not be persistent so it counts against memory limit which is the main point in changing this. But you should of course check all the usage and make sure it is never user outside the request or re-used between request - it shouldn't but better to check.

@bukka
Copy link
Member

bukka commented Jan 19, 2025

I'm also not sure about the glob limits (for malloc and others). It might be good to check if glibc have similar limits to see if they should be kept - doesn't need to be changed as part of this PR but would make sense to look into before switching to it on Linux as well.

Removes the vendored OpenBSD reallocarray and changes libc memory
management to ZendMM.

Not sure if non-persistent is the right call, I think glob users use it
then call globfree ASAP within a request, but it might be preferable to
use persistent allocations.
@NattyNarwhal
Copy link
Member Author

NattyNarwhal commented Jan 20, 2025

About the persistence, it should not be persistent so it counts against memory limit which is the main point in changing this. But you should of course check all the usage and make sure it is never user outside the request or re-used between request - it shouldn't but better to check.

In the places glob is used:

ext/ffi/ffi.c:  ret = glob(filename, 0, NULL, &globbuf);
ext/opcache/zend_accelerator_blacklist.c:       ret = glob(filename, 0, NULL, &globbuf);
ext/standard/dir.c:     if (0 != (ret = glob(pattern, flags & GLOB_FLAGMASK, NULL, &globbuf))) {
ext/zip/php_zip.c:      if (0 != (ret = glob(pattern, flags & GLOB_FLAGMASK, NULL, &globbuf))) {
main/streams/glob_wrapper.c:    if (0 != (ret = glob(pattern, pglob->flags & GLOB_FLAGMASK, NULL, &pglob->glob))) {
sapi/fpm/fpm/fpm_conf.c:                if ((i = glob(inc, GLOB_ERR | GLOB_MARK, NULL, &g)) != 0) {

...all instances of it call globfree in the same function, with the exception of the glob stream wrapper.

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