On Sat, Aug 24, 2024 at 08:41:30AM +0200, Christian Schulte wrote:
> 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?

OpenBSD mail(1) has no "adding headers" option as other versions of
mailx.  That can't happen in this case.


> 
> >  	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.

This will fail only if the code is bad written.  In this case there's
not even a variable input lenght.  What about those checks used with
malloc()?

In previos versions of my diffs I didn't do these checks, I included
them trying to follow the "secure" way things are preferred in this
project.


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

Idem above.


> 
> > +
> > +	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.

I read this but I still don't fully understand it.  I'll take a look.

Anyways, this is the adaptation I wrote to Linux, where gethostname()
prints only the hostname and POSIX HOST_NAME_MAX limited to only 64
characters.  Personally I'd prefer my non-portable version.


> 
> > +
> > +	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.

Same as the other.


> 
> > +
> > +	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,

Did you try do that with other command line MUAs, Mutt for example?


> 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,

You say that OpenSMTPD is also doing it wrong?

> 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.

Is there some command line MUA that behave in the way you're asking?


> Frankly, Message-ID alone, without References and In-Reply-To
> does not add any value.

You need to read the whole history (I don't advise you to do it :-)).
They asked me to split my diff and poste my proposal in chunks.  You can
download my patches and take a look to my whole proposal:

  https://en.roquesor.com/Downloads/mail_patches.tar.gz


> 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.

I'd love to be as sure as you seem to be about what they want. :-)

> 
> -- 
> Christian
> 
> 

Thank Christian you for your suggestions!


-- 
Walter