mail: fix exit code handling of sendmail cmd

[lollipopman]

Prior to this commit the return code of the pclose function was assumed to be the exit code of the process. However, the returned value as specified in wait(2) is a bit packed integer and must be interpreted with the provided macros. After this commit we use the macros to obtain the status code and also add some logging on failure.

For WIN32 we only check if the return value is non-zero, which should solve, #43327

[lollipopman]

@bukka would love a review of this patch, if you have the time, thanks

[TimWolla]

Emitting a warning makes sense to me, but adding new warnings likely is not acceptable in a patch update. Can you split the bugfix and the new warnings into separate commits to make it easier to see what is part of the fix and what is “extra”?

[lollipopman]

@TimWolla thanks for looking at the patch, I've split it into two commits, let me know if that reads better, thanks!

[TimWolla]

For WIN32 we only check if the return value is non-zero, which should solve, #43327

This is a non-existent reference.

---

Can you also clarify what the consequences of the erroneous check where? Did mail() sometimes return false in success cases or true in error cases?

[TimWolla]

Is it intentional that this is not also running for Windows? The commit message is not entirely clear on that and I lack the Windows expertise.

[bukka]

Windows use c client so it doesn't spawn external process.

[bukka]

I need to actually re-read it how it works - we have got a smtp implementation but seems like it's using popen_ex in some cases.

[lollipopman]

Windows doesn't inlude, sysexits.h where these are defined, so that is intentional.

[bukka]

Ok, it would need sendmail path to be specified but the default is NULL is it will usually use the c client but there is a possibility it might use it. In terms of this check I suppose that it won't probably report error in pclose so it's just ignored.

[bukka]

Yeah it's actually just check for -1 as document in https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/pclose?view=msvc-170

[lollipopman]

From my read of that documentation, the return value is -1, if _pclose fails, otherwise it is the return value of the process. So if _pclose returns something other than 0, we should also fail.

[lollipopman]

For WIN32 we only check if the return value is non-zero, which should solve, #43327

This is a non-existent reference.

sorry, added a link to the old bug tracker, https://bugs.php.net/bug.php?id=43327

Can you also clarify what the consequences of the erroneous check where? Did mail() sometimes return false in success cases or true in error cases?

Updated the commit message to clarify

[bukka]

I don't think we can add those warning to 8.3 so it needs to go to different PR targeting just master.

[lollipopman]

I don't think we can add those warning to 8.3 so it needs to go to different PR targeting just master.

sounds good, I dropped the second commit, I'll post a new PR to master, if this one is accepted.

[bukka]

Hmm I'm just reading the manual for pclose ( https://man7.org/linux/man-pages/man3/popen.3.html ) and it says:

pclose(): on success, returns the exit status of the command; if
wait4(2) returns an error, or some other error is detected, -1 is returned.

That's for Linux. However FreeBSD docs ( https://man.freebsd.org/cgi/man.cgi?query=pclose&sektion=3&manpath=FreeBSD+14.2-RELEASE+and+Ports ) say this:

The pclose() function returns -1 if stream is not associated with a "popened" command, if stream already "pclosed", or if wait4(2) returns an error.

So that would match with your description.

Are you experience this issue only on BSD platform or is Linux working differently than documented?

[bukka]

Tried to read glibc source and it's using posix_spawn for popen but doen't see exactly the logic for return code in pclose which just uses normal IO way of closing the FILE. Would need to spend more time on it to see.

In any case not sure if we should merge anything not matching the docs as there might be other variant. Maybe if it's FreeBSD only, it should be fine.

[lollipopman]

I'm testing on a Debian Linux box, the documentation is a bit hard to parse, but on the Linux man page you referenced the last paragraph of the description says:

The pclose() function waits for the associated process to
terminate and returns the exit status of the command as returned
by wait4(2).

Which ultimately leads to man 2 wait as being the return value, the same as the BSDs.

Here is small program to test the behavior:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main() {
  FILE *pipe;
  int wstatus;

  pipe = popen("exit 75", "r");

  if (pipe == NULL) {
    perror("popen failed");
    exit(EXIT_FAILURE);
  }

  wstatus = pclose(pipe);

  if (wstatus == -1) {
    perror("pclose failed");
    exit(EXIT_FAILURE);
  } else {
    printf("pclose status %d\n", wstatus);
    if (WIFEXITED(wstatus)) {
      printf("WEXITSTATUS status: %d\n", WEXITSTATUS(wstatus));
    } else if (WIFSIGNALED(wstatus)) {
      printf("child killed by signal %d (%s)\n", WTERMSIG(wstatus),
             strsignal(WTERMSIG(wstatus)));
    } else {
      printf("unknown status %x\n", wstatus);
    }
  }
  return EXIT_SUCCESS;
}

[TimWolla]

I was also confused by the terminology in the man-page on Ubuntu, but at least the Musl implementation also just passes along the value retrieved from wait4:

https://github.com/esmil/musl/blob/194f9cf93da8ae62491b7386edf481ea8565ae4e/src/stdio/pclose.c

So the implementation seems correct to me.

[lollipopman]

thanks @TimWolla the musl source code is unbelievably readable!

[bukka]

Yeah looks like you are right. The docs are quite confusing though (although the description paragraph that I didn't initially read makes it more clear) so not surprised that this was implemented in this way.

[TimWolla]

LGTM. Thank you for the quick turn-around on my questions.

[bukka]

I got a little concern that we have some buggy usage that send email and sendmail might not finish correctly so I think we should be extra careful here. The problem is that we have got variation of clients and couple of last fixes caused lots of complaints. I actually tend to target master only for everything but need to think about it.

[bukka]

Or maybe we could try 8.4 and see.

[bukka]

Actually I would go for master only. As I said it might still work in some cases and our currently semantic is more just about failure from pclose is error so it's something that we should also not in UPGRADING I think.

[lollipopman]

Actually I would go for master only. As I said it might still work in some cases and our currently semantic is more just about failure from pclose is error so it's something that we should also not in UPGRADING I think.

definitely your call, I think it is fairly safe, but happy to rebase against master, if you would prefer.

[lollipopman]

Thanks for you help @bukka, alternative pull request against master, #18786