Skip to content

Commit

Permalink
Fix #77821: Potential heap corruption in TSendMail()
Browse files Browse the repository at this point in the history 
`zend_string_tolower()` returns a copy (not a duplicate) of the given
string, if it is already in lower case.  In this case we must not not
`zend_string_free()` both strings.  The cleanest solution is to call
` zend_string_release()` on both strings, which properly handles the
refcount.
  • Loading branch information
@cmb69 @smalyshev
cmb69 authored and smalyshev committed Apr 30, 2019
1 parent 588db7c commit 6c631cc
Showing 1 changed file with 7 additions and 6 deletions.
13 changes: 7 additions & 6 deletions win32/sendmail.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,9 @@ PHPAPI int TSendMail(char *host, int *error, char **error_message,
}

if (!found) {
if (headers_lc) {
zend_string_free(headers_lc);
if (headers) {
zend_string_release(headers_trim);
zend_string_release(headers_lc);
}
*error = W32_SM_SENDMAIL_FROM_NOT_SET;
return FAILURE;
Expand All @@ -289,8 +290,8 @@ PHPAPI int TSendMail(char *host, int *error, char **error_message,
efree(RPath);
}
if (headers) {
zend_string_free(headers_trim);
zend_string_free(headers_lc);
zend_string_release(headers_trim);
zend_string_release(headers_lc);
}
/* 128 is safe here, the specifier in snprintf isn't longer than that */
if (NULL == (*error_message = ecalloc(1, HOST_NAME_LEN + 128))) {
Expand All @@ -308,8 +309,8 @@ PHPAPI int TSendMail(char *host, int *error, char **error_message,
efree(RPath);
}
if (headers) {
zend_string_free(headers_trim);
zend_string_free(headers_lc);
zend_string_release(headers_trim);
zend_string_release(headers_lc);
}
if (ret != SUCCESS) {
*error = ret;
Expand Down

2 comments on commit 6c631cc

@nikic
Copy link
Member

@nikic nikic commented on 6c631cc Apr 30, 2019

Choose a reason for hiding this comment

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

We should possibly establish a policy against use of zend_string_free outside Zend. This is not the first and not the third issues of this kind -- this optimization doesn't seem worth the trouble it causes when functions are improved to reuse strings.

@smalyshev
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's time to create a README in the source with this kind of things... Or README.STRINGS specifically.

Please sign in to comment.