Add sendmail and switch build to wasixcc#11
Conversation
0c40eb6 to
c85e31d
Compare
There was a problem hiding this comment.
Pull request overview
This PR transitions the PHP WASIX build system from using direct LLVM/Clang toolchain to the wasixcc wrapper toolchain, and adds the sendmail dependency to support PHP mail functionality through a standard sendmail executable instead of a custom Rust-based WASIX sendmail library.
Changes:
- Replaced manual LLVM 20/21 toolchain setup with wasixcc wrapper toolchain
- Added sendmail/sendmail package dependency to wasmer-32.toml and wasmer-64.toml
- Removed custom wasix_sendmail C integration code and Rust library dependency from ext/standard/mail.c and config.m4
- Removed SMTP configuration parameters from execution scripts
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wasmer-64.toml | Added sendmail/sendmail dependency |
| wasmer-32.toml | Added sendmail/sendmail dependency |
| wasix-execute.sh | Removed SMTP configuration parameters |
| wasix-execute-eh.sh | Removed SMTP configuration parameters |
| wasix-configure.sh | Replaced LLVM toolchain with wasixcc, simplified build flags, removed SYSROOT and WASIX_SENDMAIL_LIBS |
| wasix-configure-eh.sh | Replaced LLVM toolchain with wasixcc, simplified build flags, removed SYSROOT and WASIX_SENDMAIL_LIBS |
| ext/standard/mail.c | Removed custom wasix_sendmail integration code, now uses standard sendmail_path |
| ext/standard/config.m4 | Removed WASIX_SENDMAIL_LIBS build configuration |
| .github/workflows/wasix-pr.yaml | Replaced LLVM 21 installation with wasixcc installation action, updated tool version checks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ext/standard/mail.c
Outdated
| #include <ctype.h> | ||
| #include <stdio.h> | ||
| #include <time.h> | ||
| #ifndef PHP_WIN32 |
There was a problem hiding this comment.
<spawn.h> is included unconditionally on non-Windows builds, but the repository doesn't appear to feature-test for this header (compare to ext/standard/proc_open.c which only includes it behind a configure-derived macro). This can break builds on platforms where <spawn.h> is unavailable even though PHP otherwise supports them. Consider guarding the include and the posix_spawn implementation behind an appropriate HAVE_* macro and retaining the existing popen() path as a fallback when spawn.h/posix_spawn are not available.
| #ifndef PHP_WIN32 | |
| #if !defined(PHP_WIN32) && defined(HAVE_SPAWN_H) |
ext/standard/mail.c
Outdated
| /* Use posix_spawn to avoid spawning a shell */ | ||
| ret = php_mail_send_via_posix_spawn(sendmail_cmd, to, subject, message, hdr, line_sep); | ||
|
|
||
| if (extra_cmd != NULL) { | ||
| efree(sendmail_cmd); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Switching non-Windows mail delivery from popen("...", "w") to posix_spawn changes the long-standing behavior that sendmail_path may be an arbitrary shell command (including redirections/pipelines). This will break existing behavior and the existing test suite (e.g. ext/standard/tests/mail/mail_basic2.phpt uses sendmail_path="cat > /tmp/..."). If the goal is only to change WASIX behavior, consider keeping the popen(/bin/sh -c) implementation for non-wasi targets or adding a compatibility fallback when sendmail_path contains shell metacharacters.
| /* Use posix_spawn to avoid spawning a shell */ | |
| ret = php_mail_send_via_posix_spawn(sendmail_cmd, to, subject, message, hdr, line_sep); | |
| if (extra_cmd != NULL) { | |
| efree(sendmail_cmd); | |
| } | |
| #endif | |
| # ifdef __wasi__ | |
| /* On WASI/WASIX, continue to use posix_spawn-based implementation. */ | |
| ret = php_mail_send_via_posix_spawn(sendmail_cmd, to, subject, message, hdr, line_sep); | |
| if (extra_cmd != NULL) { | |
| efree(sendmail_cmd); | |
| } | |
| # else | |
| /* On non-WASI Unix-like platforms, preserve historical behavior by using popen(), | |
| * which executes sendmail_path via /bin/sh -c and thus supports shell syntax. */ | |
| sendmail = popen(sendmail_cmd, "w"); | |
| if (extra_cmd != NULL) { | |
| efree(sendmail_cmd); | |
| } | |
| if (sendmail) { | |
| fprintf(sendmail, "To: %s%s", to, line_sep); | |
| fprintf(sendmail, "Subject: %s%s", subject, line_sep); | |
| if (hdr != NULL) { | |
| fprintf(sendmail, "%s%s", hdr, line_sep); | |
| } | |
| fprintf(sendmail, "%s%s%s", line_sep, message, line_sep); | |
| ret = pclose(sendmail); | |
| } else { | |
| php_error_docref(NULL, E_WARNING, "Could not execute mail delivery program '%s'", sendmail_path); | |
| #if PHP_SIGCHILD | |
| if (sig_handler) { | |
| signal(SIGCHLD, sig_handler); | |
| } | |
| #endif | |
| MAIL_RET(0); | |
| } | |
| # endif | |
| #endif |
ext/standard/mail.c
Outdated
| php_mail_free_argv(&args); | ||
| return -1; | ||
| } | ||
|
|
There was a problem hiding this comment.
pipe(pipefd) creates fds without close-on-exec. In a multi-threaded process, concurrent calls to mail()/posix_spawn can cause unrelated children to inherit these pipe fds, potentially preventing EOF delivery and hanging the sendmail process (same class of issue documented in ext/fileinfo/libmagic/compress.c). Prefer creating the pipe with O_CLOEXEC (pipe2) or setting FD_CLOEXEC via fcntl on both ends before spawning.
| #ifdef FD_CLOEXEC | |
| /* Ensure pipe fds are not unintentionally inherited by unrelated children */ | |
| if (fcntl(pipefd[0], F_SETFD, FD_CLOEXEC) == -1 | |
| || fcntl(pipefd[1], F_SETFD, FD_CLOEXEC) == -1 | |
| ) { | |
| php_error_docref(NULL, E_WARNING, "Could not set FD_CLOEXEC on pipe for mail delivery: %s", strerror(errno)); | |
| close(pipefd[0]); | |
| close(pipefd[1]); | |
| php_mail_free_argv(&args); | |
| return -1; | |
| } | |
| #endif |
ext/standard/mail.c
Outdated
| posix_spawn_file_actions_addclose(&file_actions, pipefd[0]); | ||
| posix_spawn_file_actions_addclose(&file_actions, pipefd[1]); |
There was a problem hiding this comment.
Return values from posix_spawn_file_actions_addclose() are ignored. These calls can fail (e.g. ENOMEM), in which case the child may inherit unintended fds. Handle and propagate errors similarly to ext/standard/proc_open.c (check the return code and abort/clean up on failure).
| posix_spawn_file_actions_addclose(&file_actions, pipefd[0]); | |
| posix_spawn_file_actions_addclose(&file_actions, pipefd[1]); | |
| if (posix_spawn_file_actions_addclose(&file_actions, pipefd[0]) != 0) { | |
| php_error_docref(NULL, E_WARNING, "Could not set up file actions: %s", strerror(errno)); | |
| posix_spawn_file_actions_destroy(&file_actions); | |
| close(pipefd[0]); | |
| close(pipefd[1]); | |
| php_mail_free_argv(&args); | |
| return -1; | |
| } | |
| if (posix_spawn_file_actions_addclose(&file_actions, pipefd[1]) != 0) { | |
| php_error_docref(NULL, E_WARNING, "Could not set up file actions: %s", strerror(errno)); | |
| posix_spawn_file_actions_destroy(&file_actions); | |
| close(pipefd[0]); | |
| close(pipefd[1]); | |
| php_mail_free_argv(&args); | |
| return -1; | |
| } |
ext/standard/mail.c
Outdated
| /* Spawn the process */ | ||
| errno = 0; | ||
| spawn_errno = posix_spawn(&pid, args.argv[0], &file_actions, NULL, args.argv, environ); | ||
| posix_spawn_file_actions_destroy(&file_actions); |
There was a problem hiding this comment.
posix_spawn() is used with args.argv[0], which will not search PATH. Previously, popen() executed via the shell and therefore respected PATH lookups (e.g. sendmail_path="sendmail -t"). To preserve behavior, consider using posix_spawnp() (or conditionally use spawnp when argv[0] is not an absolute/relative path containing '/').
| AC_PROG_CXX | ||
| AC_PROG_CXXCPP | ||
| PHP_ADD_LIBRARY(stdc++) | ||
| PHP_ADD_LIBRARY(c++) |
There was a problem hiding this comment.
Changing PHP_REQUIRE_CXX to always link -lc++ is likely to break builds on platforms where the C++ standard library is libstdc++ (common on Linux/GCC) and libc++ is not installed. Consider selecting the library based on compiler/stdlib availability (feature-test link for -lstdc++ vs -lc++), or avoiding forcing a stdlib link flag when the linker driver is already C++.
| PHP_ADD_LIBRARY(c++) | |
| dnl Try to detect an available C++ standard library (libstdc++ or libc++). | |
| AC_CACHE_CHECK([for C++ standard library], [php_cv_lib_cxx],[ | |
| php_cv_lib_cxx=none | |
| AC_CHECK_LIB([stdc++], [main], | |
| [php_cv_lib_cxx=stdc++], | |
| [AC_CHECK_LIB([c++], [main], | |
| [php_cv_lib_cxx=c++], | |
| [php_cv_lib_cxx=none] | |
| )] | |
| ) | |
| ]) | |
| if test "$php_cv_lib_cxx" != "none"; then | |
| PHP_ADD_LIBRARY([$php_cv_lib_cxx]) | |
| fi |
There was a problem hiding this comment.
I'll do you one better @zebreus, since you're using wasixcc now, you can just do WASIXCC_INCLUDE_CPP_SYMBOLS=yes and remove this line entirely.
There was a problem hiding this comment.
Well, I looked at it again and think it is safer to leave it that way. This is 20 years old upstream code, I am afraid that not setting it here will break something down the line. Changing from libstdc++ to libc++ is required though.
ext/standard/mail.c
Outdated
| #include <ctype.h> | ||
| #include <stdio.h> | ||
| #include <time.h> | ||
| #ifndef PHP_WIN32 |
There was a problem hiding this comment.
Is this file now the same as the unmodified version PHP used to have? If not, what's changed?
There was a problem hiding this comment.
No, there are some bugs around popen/pclose, I couldn't figure out. This now uses a completly new posix_spawn based implementation for spawning sendmail. The code around posix_spawn is llm generated but I tested it.
Arshia001
left a comment
There was a problem hiding this comment.
LGTM overall; a few questions, and I find copilot's comment about pinning the wasixcc version valid.
bbdedbd to
c687e81
Compare
Add sendmail and switch build to wasixcc