Remove custom alloca #8513
Remove custom alloca #8513
Conversation
Girgias
force-pushed
the
alloca
branch
3 times, most recently
from
212b11d to
62d021c
Compare
ext/standard/crypt_sha256.c
Outdated
| } | ||
|
|
||
| if ((salt - (char *) 0) % __alignof__(uint32_t) != 0) { | ||
| char *tmp = (char *) alloca(salt_len + 1 + __alignof__(uint32_t)); | ||
| tmp_salt = (char *) do_alloca(salt_len + 1 + __alignof__(uint32_t), use_heap); |
There was a problem hiding this comment.
Same here, we can't reuse use_heap.
Zend/zend_portability.h
Outdated
| # define ALLOCA_FLAG(name) \ | ||
| bool name; | ||
| # define SET_ALLOCA_FLAG(name) \ | ||
| name = 1 | ||
| name = true | ||
| # define do_alloca_ex(size, limit, use_heap) \ | ||
| ((use_heap = (UNEXPECTED((size) > (limit)))) ? emalloc(size) : alloca(size)) | ||
| # define do_alloca(size, use_heap) \ |
There was a problem hiding this comment.
I'm wondering, would it make sense to not enable it in debug mode? Which would make it clearer that a memory leak can appear if one forgets the corresponding free. As currently only macOS on CI is our safeguard, and I still don't really understand why it's not enabled on macOS
There was a problem hiding this comment.
I don't know why it's not available on macOS, but the detection might not be handled properly from our side. Apparently, we doing AC_CHECK_HEADERS([alloca.h]), but later also AC_FUNC_ALLOCA, although that macro is supposed to check for alloca.h and set HAVE_ALLOCA_H, too. That might not be a real problem, though (I don't know enough about autoconf). I guess someone with access to a macOS machine would need to debug.
Anyhow, it might be a good idea to disable alloca() for debug mode.
There was a problem hiding this comment.
line 348, it s guarded with !defined(DARWIN)
There was a problem hiding this comment.
I know that, the reason is more why? I've been told apparently the behaviour on macOS and Linux is quite different.
There was a problem hiding this comment.
seems it s like this since a long time, maybe the difference is no longer true ?
Use do_alloca in place of alloca Add support for Windows
|
Think the FPM cases can be left as there's not much point to change them... |
Use do_alloca in place of alloca
Add support for Windows
There are 3 remaining usages of
allocawhich are located in FPM (fpm_sockets.candfpm_env.c) which include the header. And I'm not sure if it is OK to includezend_portability.hin it.