On 23.08.24 10:34, Walter Alejandro Iglesias wrote:

...
[snip]
...
> 
> Index: def.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/mail/def.h,v
> diff -u -p -r1.17 def.h
> --- def.h	28 Jan 2022 06:18:41 -0000	1.17
> +++ def.h	22 Aug 2024 11:02:15 -0000
> @@ -55,6 +55,9 @@
>  #include <limits.h>
>  #include <vis.h>
>  #include "pathnames.h"
> +#include <sys/types.h>
> +#include <sys/socket.h>
> +#include <netdb.h>
>  
>  #define	APPEND				/* New mail goes to end of mailbox */
>  
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/mail/extern.h,v
> diff -u -p -r1.30 extern.h
> --- extern.h	21 May 2024 05:00:48 -0000	1.30
> +++ extern.h	22 Aug 2024 11:02:15 -0000
> @@ -129,6 +129,7 @@ int	 forward(char *, FILE *, char *, int
>  void	 free_child(pid_t);
>  int	 from(void *);
>  off_t	 fsize(FILE *);
> +char*	 genmsgid(void);
>  int	 getfold(char *, int);
>  int	 gethfield(FILE *, char *, int, char **);
>  int	 gethfromtty(struct header *, int);
> Index: send.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mail/send.c,v
> diff -u -p -r1.26 send.c
> --- send.c	8 Mar 2023 04:43:11 -0000	1.26
> +++ send.c	22 Aug 2024 11:02:15 -0000
> @@ -525,6 +525,8 @@ puthead(struct header *hp, FILE *fo, int
>  		fmt("To:", hp->h_to, fo, w&GCOMMA), gotcha++;
>  	if (hp->h_subject != NULL && w & GSUBJECT)
>  		fprintf(fo, "Subject: %s\n", hp->h_subject), gotcha++;
> +	if (fo != stdout)
> +		fprintf(fo, "Message-ID: %s\n", genmsgid()), gotcha++;

Maybe this needs to check no message id header is already present?

>  	if (hp->h_cc != NULL && w & GCC)
>  		fmt("Cc:", hp->h_cc, fo, w&GCOMMA), gotcha++;
>  	if (hp->h_bcc != NULL && w & GBCC)
> Index: util.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mail/util.c,v
> diff -u -p -r1.2 util.c
> --- util.c	26 Dec 2022 19:16:01 -0000	1.2
> +++ util.c	22 Aug 2024 11:02:15 -0000
> @@ -641,3 +641,44 @@ clearnew(void)
>  		}
>  	}
>  }
> +
> +/* Generate Message-ID */
> +char*
> +genmsgid(void)
> +{
> +	size_t n = 0;
> +	char c;
> +	static char r[24];
> +	struct addrinfo hints, *info, *p;
> +	char *fqdn = NULL;
> +	static char hostname[NI_MAXHOST];
> +	static char buf[sizeof(r) + sizeof(hostname) + 4];
> +	int error = 0;
> +
> +	while (n < sizeof(r) - 1) {
> +		c = arc4random_uniform(173);
> +		if (isalnum(c) && snprintf(&r[n++], 2, "%c", c) != 1)
> +			errx(1, "snprintf");

Message-ID is an optional header. No need to terminate the whole
application, just because it cannot generate that optional header. Maybe
that should have read "genmsgid: snprintf" for consistency, as well.


> +	}
> +
> +	if (gethostname(hostname, sizeof(hostname)))
> +		errx(1, "genmsgid: gethostname");

Same here.

> +
> +	memset(&hints, 0, sizeof hints);
> +	hints.ai_family = AF_UNSPEC;
> +	hints.ai_socktype = SOCK_STREAM;
> +	hints.ai_flags = AI_CANONNAME;
> +
> +	if (getaddrinfo(hostname, NULL, &hints, &info) != 0)
> +		errx(1, "genmsgid: getaddrinfo");

And here. I think getaddrinfo is not needed and just the hostname
returned by gethostname should do the job. It's setup from /etc/myname
during boot. I am not sure but maybe this tries to perform DNS lookups
so mail could no longer be used when the system isn't connected to a
network, just to generate an optional message id header? Quite a common
usecase. If this takes away that usecase, I am all against it. But not
just because of that. See below.

> +
> +	for (p = info; p != NULL; p = p->ai_next)
> +		fqdn = p->ai_canonname;

I am not sure this is correct. The manpage of getaddrinfo has this:

If the AI_CANONNAME bit is set, a successful call to getaddrinfo() will
return a NUL-terminated string containing the canonical name of the
specified host name in the ai_canonname element of the *first* addrinfo
structure returned.

I would take that "first" seriously. Maybe someone familiar with the asr
code implementing getaddrinfo can comment on that. Also this is lacking
NULL checks as well.

> +
> +	error = snprintf(buf, sizeof(buf), "<%s@%s>", r, fqdn);
> +	if (error < 0 || error >= sizeof(buf))
> +		errx(1, "genmsgid: snprintf");

Optional header, do not terminate the whole application due to this.

> +
> +	freeaddrinfo(info);
> +	return(buf);
> +}
> 

Re-reading <https://datatracker.ietf.org/doc/html/rfc2822#section-3.6.4>
I think there is no value in just blindly adding a message id header.
You cannot tell if the user intentionally did not add that header, so
this could be rated a regression for some users.

For example:

for i in `cat /etc/lots_of_recipients`;do echo "Hell of a message." |
mail $i;done

If mail would silently add a different message-id header to all of those
messages, it would certainly behave incorrectly, as that is the same
instance of a message sent to multiple recipients from e.g. a script. In
my opinion, I would not want mail to start adding that header after an
upgrade, the same way I would not want OpenSMTPD to blindly start adding
message ids, although it knows nothing about the message or what the
author intended to do. If someone replies to one of those messages above
to all recipients from /etc/lots_of_recipients, the MUA could not sort
out about it to be the exact same message instance replied to, for
example. Frankly, Message-ID alone, without References and In-Reply-To
does not add any value. Sorry to say. Not an OpenBSD developer. Just
reading some of theire mailing lists. I am pretty sure, none of your
patches will ever get committed that way.

-- 
Christian