Skip to content

win32/sendmail.c: Various refactorings (part 2) #20789

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
Girgias wants to merge 9 commits into
base: master
Choose a base branch
from
+107 −126
Open

win32/sendmail.c: Various refactorings (part 2) #20789

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2943526
win32/sendmail.c/TSendMail(): use bool type instead of int
Girgias Dec 27, 2025
ccf8e3f
win32/sendmail.c/GetAddr(): use standard char* type
Girgias Dec 27, 2025
28fa7b6
win32/sendmail.c: add const qualifiers
Girgias Dec 27, 2025
04c96de
win32/sendmail.c/MailConnect(): remove outdated comment
Girgias Dec 27, 2025
aab2c2b
win32/sendmail.c/MailConnect(): move socket creation
Girgias Dec 27, 2025
708144b
win32/sendmail.c: move initial MailConnect() into SendText()
Girgias Dec 27, 2025
b51c2cd
win32/sendmail.c/MailConnect(): pass host as param
Girgias Dec 27, 2025
8cff225
win32/sendmail.c/SendText(): mark RPath argument as _In_
Girgias Dec 27, 2025
70eb9f5
win32/sendmail.c: create function for SMTP HELO command
Girgias Dec 27, 2025
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
127 changes: 52 additions & 75 deletions win32/sendmail.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,14 @@ static const char *ErrorMessages[] =
#define PHP_WIN32_MAIL_DOT_PATTERN "\n."
#define PHP_WIN32_MAIL_DOT_REPLACE "\n.."

static int SendText(char *RPath, const char *Subject, const char *mailTo, const char *data,
static int SendText(_In_ const char *host, const char *RPath, const char *Subject, const char *mailTo, const char *data,
Copy link
Member

@ndossche ndossche Dec 28, 2025

Choose a reason for hiding this comment

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

Why did you add a SAL here?

Copy link
Member Author

@Girgias Girgias Dec 28, 2025

Choose a reason for hiding this comment

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

It seems this is how you'd add the __attribute__((nonnull(1))); from GCC but for Windows/MSVC, if there is another way happy to take it.

Copy link
Member

@ndossche ndossche Dec 28, 2025

Choose a reason for hiding this comment

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

I would need to look it up, but I don't think that's true? But it's been a while since I saw SAL.

Copy link
Member Author

@Girgias Girgias Dec 28, 2025

Choose a reason for hiding this comment

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

Well I got there by trying to navigate MS's docs... but if you find the correct thing than I'm happy to change it.

zend_string *headers, zend_string *headers_lc, char **error_message);
static int MailConnect();
static bool PostHeader(char *RPath, const char *Subject, const char *mailTo, zend_string *xheaders);
static bool PostHeader(const char *RPath, const char *Subject, const char *mailTo, zend_string *xheaders);
static bool Post(LPCSTR msg);
static int Ack(char **server_response);
static unsigned long GetAddr(LPSTR szHost);
static int FormatEmailAddress(char* Buf, char* EmailAddress, char* FormatString);
static unsigned long GetAddr(const char *szHost);
static int FormatEmailAddress(char* Buf, const char* EmailAddress, const char* FormatString);

/* This function is meant to unify the headers passed to to mail()
* This means, use PCRE to transform single occurrences of \n or \r in \r\n
Expand Down Expand Up @@ -196,11 +196,6 @@ PHPAPI int TSendMail(const char *host, int *error, char **error_message,
if (host == NULL) {
*error = BAD_MAIL_HOST;
return FAILURE;
} else if (strlen(host) >= HOST_NAME_LEN) {
*error = BAD_MAIL_HOST;
return FAILURE;
} else {
strcpy(PW32G(mail_host), host);
}

if (headers) {
Expand All @@ -219,7 +214,7 @@ PHPAPI int TSendMail(const char *host, int *error, char **error_message,
if (INI_STR("sendmail_from")) {
RPath = estrdup(INI_STR("sendmail_from"));
} else if (headers_lc) {
int found = 0;
bool found = false;
const char *lookup = ZSTR_VAL(headers_lc);

while (lookup) {
Expand All @@ -236,7 +231,7 @@ PHPAPI int TSendMail(const char *host, int *error, char **error_message,
}
}

found = 1;
found = true;

/* Real offset is memaddress from the original headers + difference of
* string found in the lowercase headers + 5 characters to jump over
Expand All @@ -261,39 +256,20 @@ PHPAPI int TSendMail(const char *host, int *error, char **error_message,
}
}

/* attempt to connect with mail host */
Copy link
Member

@ndossche ndossche Dec 28, 2025

Choose a reason for hiding this comment

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

I think this commit is not very useful code motion that complicates things to be honest. If it works why change it? It's not like this is a maintainability problem.
I also don't want to diverge too much from the original wSendmail source code.

Copy link
Member Author

@Girgias Girgias Dec 28, 2025

Choose a reason for hiding this comment

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

I cannot find how the wSendmail code would even look like as everything is dead links and google has become trash and is unhelpful. So I can't even compare how much we have diverged already and if this not just "we maintain this now"

Copy link
Member

@ndossche ndossche Dec 28, 2025

Choose a reason for hiding this comment

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

Fair enough, although I wish reality was different, I feel like we already maintain too many parts ourselves.

Copy link
Member Author

@Girgias Girgias Dec 28, 2025

Choose a reason for hiding this comment

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

I very much agree with this, also having the whole mail functionality chucked away in a small external extension would also solve this problem of not needing to maintain this ourself :/

Copy link
Member

@kallesommernielsen kallesommernielsen Dec 29, 2025

Choose a reason for hiding this comment

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

I can think of two points in the past where we deviated from the original code:

  1. Around 5.3 when we made use of pcre over ereg as ext/pcre became always available

  2. When I many years ago fixed it to work owing IPv6 and intranet domains (specifically the HELO part)

So we have already stepped over that edge a long time ago, either way I have no opinion on the matter just some historical context

*error = MailConnect();
if (*error != 0) {
if (RPath) {
efree(RPath);
}
if (headers) {
zend_string_release(headers_trim);
zend_string_release(headers_lc);
}
/* 128 is safe here, the specifier in snprintf isn't longer than that */
*error_message = ecalloc(1, HOST_NAME_LEN + 128);
snprintf(*error_message, HOST_NAME_LEN + 128,
"Failed to connect to mailserver at \"%s\" port " ZEND_ULONG_FMT ", verify your \"SMTP\" "
"and \"smtp_port\" setting in php.ini or use ini_set()",
PW32G(mail_host), !INI_INT("smtp_port") ? 25 : INI_INT("smtp_port"));
ret = SendText(host, RPath, Subject, mailTo, data, headers_trim, headers_lc, error_message);
TSMClose();
if (RPath) {
efree(RPath);
}
if (headers) {
zend_string_release(headers_trim);
zend_string_release(headers_lc);
}
if (ret != SUCCESS) {
*error = ret;
return FAILURE;
} else {
ret = SendText(RPath, Subject, mailTo, data, headers_trim, headers_lc, error_message);
TSMClose();
if (RPath) {
efree(RPath);
}
if (headers) {
zend_string_release(headers_trim);
zend_string_release(headers_lc);
}
if (ret != SUCCESS) {
*error = ret;
return FAILURE;
}
return SUCCESS;
}
return SUCCESS;
}

//*********************************************************************
Expand Down Expand Up @@ -387,7 +363,7 @@ static char *find_address(char *list, char **state)
// Author/Date: jcar 20/9/96
// History:
//*********************************************************************
static int SendText(char *RPath, const char *Subject, const char *mailTo, const char *data,
static int SendText(_In_ const char *host, const char *RPath, const char *Subject, const char *mailTo, const char *data,
zend_string *headers, zend_string *headers_lc, char **error_message)
{
int res;
Expand Down Expand Up @@ -415,19 +391,29 @@ static int SendText(char *RPath, const char *Subject, const char *mailTo, const
return (BAD_MSG_DESTINATION);
*/

if (strlen(host) >= HOST_NAME_LEN) {
*error = BAD_MAIL_HOST;
return FAILURE;
} else {
strcpy(PW32G(mail_host), host);
}

/* attempt to connect with mail host */
res = MailConnect();
if (res != 0) {
/* 128 is safe here, the specifier in snprintf isn't longer than that */
*error_message = ecalloc(1, HOST_NAME_LEN + 128);
snprintf(*error_message, HOST_NAME_LEN + 128,
"Failed to connect to mailserver at \"%s\" port " ZEND_ULONG_FMT ", verify your \"SMTP\" "
"and \"smtp_port\" setting in php.ini or use ini_set()",
host, !INI_INT("smtp_port") ? 25 : INI_INT("smtp_port"));
return res;
}

snprintf(PW32G(mail_buffer), sizeof(PW32G(mail_buffer)), "HELO %s\r\n", PW32G(mail_local_host));

/* in the beginning of the dialog */
/* attempt reconnect if the first Post fail */
if (!Post(PW32G(mail_buffer))) {
int err = MailConnect();
if (0 != err) {
return (FAILED_TO_SEND);
}

if (!Post(PW32G(mail_buffer))) {
return (FAILED_TO_SEND);
}
return (FAILED_TO_SEND);
}
if ((res = Ack(&server_response)) != SUCCESS) {
SMTP_ERROR_RESPONSE(server_response);
Expand Down Expand Up @@ -658,7 +644,7 @@ static int SendText(char *RPath, const char *Subject, const char *mailTo, const
// Author/Date: jcar 20/9/96
// History:
//*********************************************************************
static bool PostHeader(char *RPath, const char *Subject, const char *mailTo, zend_string *xheaders)
static bool PostHeader(const char *RPath, const char *Subject, const char *mailTo, zend_string *xheaders)
{
/* Print message header according to RFC 822 */
/* Return-path, Received, Date, From, Subject, Sender, To, cc */
Expand Down Expand Up @@ -738,21 +724,14 @@ static int MailConnect()
return 0;
#endif

/* Create Socket */
if ((PW32G(mail_socket) = socket(PF_INET, SOCK_STREAM, 0)) == INVALID_SOCKET) {
return (FAILED_TO_OBTAIN_SOCKET_HANDLE);
}

/* Get our own host name */
if (gethostname(PW32G(mail_local_host), HOST_NAME_LEN)) {
closesocket(PW32G(mail_socket));
return (FAILED_TO_GET_HOSTNAME);
}

ent = gethostbyname(PW32G(mail_local_host));

if (!ent) {
closesocket(PW32G(mail_socket));
return (FAILED_TO_GET_HOSTNAME);
}

Expand All @@ -765,7 +744,6 @@ return 0;
#endif
{
if (namelen + 2 >= HOST_NAME_LEN) {
closesocket(PW32G(mail_socket));
return (FAILED_TO_GET_HOSTNAME);
}

Expand All @@ -774,20 +752,19 @@ return 0;
strcpy(PW32G(mail_local_host) + namelen + 1, "]");
} else {
if (namelen >= HOST_NAME_LEN) {
closesocket(PW32G(mail_socket));
return (FAILED_TO_GET_HOSTNAME);
}

strcpy(PW32G(mail_local_host), ent->h_name);
}

/* Resolve the servers IP */
/*
if (!isdigit(PW32G(mail_host)[0])||!gethostbyname(PW32G(mail_host)))
{
return (FAILED_TO_RESOLVE_HOST);
/* Create Socket */
if ((PW32G(mail_socket) = socket(PF_INET, SOCK_STREAM, 0)) == INVALID_SOCKET) {
return (FAILED_TO_OBTAIN_SOCKET_HANDLE);
}
*/

/* Resolve the servers IP */
unsigned long server_addr = GetAddr(PW32G(mail_host));

portnum = (short) INI_INT("smtp_port");
if (!portnum) {
Expand All @@ -797,7 +774,7 @@ return 0;
/* Connect to server */
sock_in.sin_family = AF_INET;
sock_in.sin_port = htons(portnum);
sock_in.sin_addr.S_un.S_addr = GetAddr(PW32G(mail_host));
sock_in.sin_addr.S_un.S_addr = server_addr;

if (connect(PW32G(mail_socket), (LPSOCKADDR) & sock_in, sizeof(sock_in))) {
closesocket(PW32G(mail_socket));
Expand Down Expand Up @@ -904,7 +881,7 @@ static int Ack(char **server_response)


//*********************************************************************
// Name: unsigned long GetAddr (LPSTR szHost)
// Name: unsigned long GetAddr (const char *szHost)
// Input:
// Output:
// Description: Given a string, it will return an IP address.
Expand All @@ -915,7 +892,7 @@ static int Ack(char **server_response)
// Author/Date: jcar 20/9/96
// History:
//*********************************************************************
static unsigned long GetAddr(LPSTR szHost)
static unsigned long GetAddr(const char *szHost)
{
LPHOSTENT lpstHost;
u_long lAddr = INADDR_ANY;
Expand All @@ -941,10 +918,10 @@ static unsigned long GetAddr(LPSTR szHost)
} /* end GetAddr() */

/* returns the contents of an angle-addr (caller needs to efree) or NULL */
static char *get_angle_addr(char *address)
static char *get_angle_addr(const char *address)
{
bool in_quotes = 0;
char *p1 = address, *p2;
const char *p1 = address, *p2;

while ((p1 = strpbrk(p1, "<\"\\")) != NULL) {
if (*p1 == '\\' && in_quotes) {
Expand Down Expand Up @@ -994,7 +971,7 @@ static char *get_angle_addr(char *address)
// Author/Date: garretts 08/18/2009
// History:
//*********************************************************************
static int FormatEmailAddress(char* Buf, char* EmailAddress, char* FormatString) {
static int FormatEmailAddress(char* Buf, const char* EmailAddress, const char* FormatString) {
char *tmpAddress;
int result;

Expand Down