Skip to content

Implement GH-15483: Use C23 memset_explicit() for ZEND_SECURE_ZERO() if available #18713

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement GH-15483: Use C23 memset_explicit() for ZEND_SECURE_ZERO() if available #18713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Zend/zend_portability.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,8 @@ extern "C++" {

#ifdef ZEND_WIN32
#define ZEND_SECURE_ZERO(var, size) RtlSecureZeroMemory((var), (size))
#elif defined(HAVE_MEMSET_EXPLICIT)
#define ZEND_SECURE_ZERO(var, size) memset_explicit((var), 0, (size))
Comment on lines 492 to +495
Copy link
Member

Choose a reason for hiding this comment

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

Should these be reordered? I would hope that Windows also gets memset_explicit() and then it should be the preferred one IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think relying on the RTL is bad. Also: we're talking about Windows, from the company that can't even make a C99 compliant compiler nor preprocessor

Copy link
Member

Choose a reason for hiding this comment

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

that can't even make a C99 compliant compiler nor preprocessor

I think this is due to VLA which have been removed from the standard since then?

Basically what I'm hoping for with the question is that we can do away with the ZEND_SECURE_ZERO macro one day and just use memset_explicit() directly everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is due to VLA which have been removed from the standard since then?

No, I was referring (non-limitative) to horrors like https://learn.microsoft.com/en-us/cpp/build/reference/experimental-preprocessor?view=msvc-170

Basically what I'm hoping for with the question is that we can do away with the ZEND_SECURE_ZERO macro one day and just use memset_explicit() directly everywhere.

I get that, but it would take years before we can bump to C23 anyway

#else
#define ZEND_SECURE_ZERO(var, size) explicit_bzero((var), (size))
#endif
Expand Down
1 change: 1 addition & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,7 @@ AC_CHECK_FUNCS(m4_normalize([
memmem
mempcpy
memrchr
memset_explicit
mkstemp
mmap
nice
Expand Down
6 changes: 4 additions & 2 deletions main/explicit_bzero.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@

PHPAPI void php_explicit_bzero(void *dst, size_t siz)
{
#ifdef HAVE_EXPLICIT_MEMSET
explicit_memset(dst, 0, siz);
#ifdef HAVE_MEMSET_EXPLICIT /* C23 */
memset_explicit(dst, 0, siz);
#elif defined(HAVE_EXPLICIT_MEMSET) /* NetBSD-specific */
explicit_memset(dst, 0, siz);
#elif defined(PHP_WIN32)
RtlSecureZeroMemory(dst, siz);
#elif defined(__GNUC__)
Expand Down