git-send-email generates mail with invalid Message-Id

git-send-email generates mail with invalid Message-Id

[Frans Pop]

I follow lkml through a local news server (inn2), using a mail2news script 
to convert incoming mails to news items.

Occasionally I get the following error in my system logs:
innd: localhost:18 bad_messageid <12487185672026-git-send-email->

The problem is that a Message-Id is supposed (RFC 822) to end in a domain 
part (...@example.com), but that's missing.

I assume that this is a configuration issue in the git setup of the 
sender, but shouldn't git-send-email refuse to send out messages with an 
invalid Message-Id?

Cheers,
FJP

Re: git-send-email generates mail with invalid Message-Id

[Erik Faye-Lund]

On Tue, Jul 28, 2009 at 4:46 AM, Frans Pop<elendil@planet.nl> wrote:
> The problem is that a Message-Id is supposed (RFC 822) to end in a domain
> part (...@example.com), but that's missing.

Correct.

> I assume that this is a configuration issue in the git setup of the
> sender, but shouldn't git-send-email refuse to send out messages with an
> invalid Message-Id?

Not quite. git-send-email generates these message-ids itself (those
who contain "-git-send-email-", that is), and should as such be able
to rely on them being generated correctly. However, I'm a bit curious
as to how these ends up incorrect in the first place. The code in
make_message_id tries to append the sender's e-mail to
timestamp+"-git-send-email-", if that fails it tries the comitter's
e-mail, then the author's e-mail. As a last resort, it tries to append
"user@"+hostname.

I'm no perl-expert, but the code looks pretty much correct to me.

The problematic e-mails, are they coming from another user than you?
Can you find out who that is, and check what git-version he or she
runs?

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: git-send-email generates mail with invalid Message-Id

[Thomas Rast]

Erik Faye-Lund wrote:
> On Tue, Jul 28, 2009 at 4:46 AM, Frans Pop<elendil@planet.nl> wrote:
> > I assume that this is a configuration issue in the git setup of the
> > sender, but shouldn't git-send-email refuse to send out messages with an
> > invalid Message-Id?
> 
> Not quite. git-send-email generates these message-ids itself (those
> who contain "-git-send-email-", that is), and should as such be able
> to rely on them being generated correctly. [...]
> I'm no perl-expert, but the code looks pretty much correct to me.

git-format-patch generates its own message IDs if it needs them for
threading, with gen_message_id() (in builtin-log.c).  That one appends
the committer email address blindly, without verifying that it has an
@ in it.

Blame the committer's broken config, I guess.  The untested patch at
the end might catch this, but then it's still a fair ways from correct
address verification _and_ email addresses aren't required to have a
hostname part.

diff --git i/builtin-log.c w/builtin-log.c
index fe8e4e1..7003784 100644
--- i/builtin-log.c
+++ w/builtin-log.c
@@ -604,9 +604,12 @@ static void gen_message_id(struct rev_info *info, char *base)
 	const char *committer = git_committer_info(IDENT_WARN_ON_NO_NAME);
 	const char *email_start = strrchr(committer, '<');
 	const char *email_end = strrchr(committer, '>');
+	const char *email_at = strrchr(committer, '@');
 	struct strbuf buf = STRBUF_INIT;
 	if (!email_start || !email_end || email_start > email_end - 1)
 		die("Could not extract email from committer identity.");
+	if (!email_at || email_start > email_at - 1 || email_at > email_end - 1)
+		die ("Committer email address invalid, cannot form message-id");
 	strbuf_addf(&buf, "%s.%lu.git.%.*s", base,
 		    (unsigned long) time(NULL),
 		    (int)(email_end - email_start - 1), email_start + 1);

Re: git-send-email generates mail with invalid Message-Id

[Erik Faye-Lund]

On Tue, Jul 28, 2009 at 11:27 AM, Thomas Rast<trast@student.ethz.ch> wrote:
> Erik Faye-Lund wrote:
>> On Tue, Jul 28, 2009 at 4:46 AM, Frans Pop<elendil@planet.nl> wrote:
>> > I assume that this is a configuration issue in the git setup of the
>> > sender, but shouldn't git-send-email refuse to send out messages with an
>> > invalid Message-Id?
>>
>> Not quite. git-send-email generates these message-ids itself (those
>> who contain "-git-send-email-", that is), and should as such be able
>> to rely on them being generated correctly. [...]
>> I'm no perl-expert, but the code looks pretty much correct to me.
>
> git-format-patch generates its own message IDs if it needs them for
> threading, with gen_message_id() (in builtin-log.c).  That one appends
> the committer email address blindly, without verifying that it has an
> @ in it.

That must be a separate issue (but quite possibly a valid one), since
gen_message_id's ids dont' contain "-send-email-" like the message-id
in the report does, no?

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: git-send-email generates mail with invalid Message-Id

[Thomas Rast]

Erik Faye-Lund wrote:
> On Tue, Jul 28, 2009 at 11:27 AM, Thomas Rast<trast@student.ethz.ch> wrote:
> > git-format-patch generates its own message IDs if it needs them for
> > threading, with gen_message_id() (in builtin-log.c).  That one appends
> > the committer email address blindly, without verifying that it has an
> > @ in it.
> 
> That must be a separate issue (but quite possibly a valid one), since
> gen_message_id's ids dont' contain "-send-email-" like the message-id
> in the report does, no?

Ah, true of course.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: git-send-email generates mail with invalid Message-Id

[Frans Pop]

On Tuesday 28 July 2009, Erik Faye-Lund wrote:
> The problematic e-mails, are they coming from another user than you?
> Can you find out who that is, and check what git-version he or she
> runs?

The Message-Id in my mail was an actual example. Luckily a search for it 
gave the following link: http://patchwork.kernel.org/patch/37597/

See the link for the name of the user. The headers have:
   X-Mailer: git-send-email 1.5.2.5

Cheers,
FJP

Re: git-send-email generates mail with invalid Message-Id

[Frans Pop]

On Tuesday 28 July 2009, Frans Pop wrote:
> On Tuesday 28 July 2009, Erik Faye-Lund wrote:
> > The problematic e-mails, are they coming from another user than you?
> > Can you find out who that is, and check what git-version he or she
> > runs?
>
> The Message-Id in my mail was an actual example. Luckily a search for
> it gave the following link: http://patchwork.kernel.org/patch/37597/
>
> See the link for the name of the user. The headers have:
>    X-Mailer: git-send-email 1.5.2.5

And here are two variants of the full thread containing that patch:
http://lkml.org/lkml/2009/7/27/274
http://groups.google.com/group/linux.kernel/browse_thread/thread/49acfc484d261758

In the second the original messages (incl. headers) can be seen, but you 
have to look for X-Original-... because of Google mail2news conversion.

Re: git-send-email generates mail with invalid Message-Id

[Nicolas Sebrecht]

Erik Faye-Lund wrote:
> On Tue, Jul 28, 2009 at 4:46 AM, Frans Pop<elendil@planet.nl> wrote:
>
> > I assume that this is a configuration issue in the git setup of the
> > sender, but shouldn't git-send-email refuse to send out messages with an
> > invalid Message-Id?

Stricly speacking, it is not an invalid Message-Id. RFC 2822 says that
the Message-Id has to be unique. The right hand side may not contain a
domain identifier. It is a RECOMMENDED practice (a good one, though).

IMHO, inn2 does a wrong assumption.

> Not quite. git-send-email generates these message-ids itself (those
> who contain "-git-send-email-", that is), and should as such be able
> to rely on them being generated correctly. [...]
> I'm no perl-expert, but the code looks pretty much correct to me.

Looks good here too. That said, if $du_part is still empty after all the
stuff over, we could add a fake domain name. This prevent the Message-Id
from ending with "-git-send-email->".

---
This is untested.

 git-send-email.perl |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index d508f83..82fb3b9 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -748,6 +748,9 @@ sub make_message_id
                use Sys::Hostname qw();
                $du_part = 'user@' . Sys::Hostname::hostname();
        }
+       if (not defined $du_part or $du_part eq '') {
+               $du_part = 'git@fake.dom';
+       }
        my $message_id_template = "<%s-git-send-email-%s>";
        $message_id = sprintf($message_id_template, $uniq, $du_part);
        #print "new message id = $message_id\n"; # Was useful for debugging
-- 
Nicolas Sebrecht

Re: git-send-email generates mail with invalid Message-Id

[Frans Pop]

On Tuesday 28 July 2009, you wrote:
> Erik Faye-Lund wrote:
> > On Tue, Jul 28, 2009 at 4:46 AM, Frans Pop<elendil@planet.nl> wrote:
> > > I assume that this is a configuration issue in the git setup of the
> > > sender, but shouldn't git-send-email refuse to send out messages
> > > with an invalid Message-Id?
>
> Stricly speacking, it is not an invalid Message-Id. RFC 2822 says that
> the Message-Id has to be unique. The right hand side may not contain a
> domain identifier. It is a RECOMMENDED practice (a good one, though).

It also says that (3.6.4):
   The message identifier (msg-id) is similar in syntax to an angle-addr
   construct without the internal CFWS.

And defines:
   message-id      =       "Message-ID:" msg-id CRLF
   msg-id          =       [CFWS] "<" id-left "@" id-right ">" [CFWS]

So, the domain part *is* required (or at least: there has to be a "@"; it 
maybe does not require id-right to be an actual domain, but that's not 
really relevant here).

So, IMO inn2's check is correct.

Re: git-send-email generates mail with invalid Message-Id

[Nicolas Sebrecht]

The 28/07/09, Frans Pop wrote:
> On Tuesday 28 July 2009, you wrote:
> > Erik Faye-Lund wrote:
> >
> > Stricly speacking, it is not an invalid Message-Id. RFC 2822 says that
> > the Message-Id has to be unique. The right hand side may not contain a
> > domain identifier. It is a RECOMMENDED practice (a good one, though).
> 
> It also says that (3.6.4):
>    The message identifier (msg-id) is similar in syntax to an angle-addr
>    construct without the internal CFWS.
> 
> And defines:
>    message-id      =       "Message-ID:" msg-id CRLF
>    msg-id          =       [CFWS] "<" id-left "@" id-right ">" [CFWS]
> 
> So, the domain part *is* required (or at least: there has to be a "@"; it 
> maybe does not require id-right to be an actual domain, but that's not 
> really relevant here).
> 
> So, IMO inn2's check is correct.

Hum, you're right. The '@' symbol is required, whatever "id-right" is.
My previous patch should fix it.

-- 
Nicolas Sebrecht

Re: git-send-email generates mail with invalid Message-Id

[Erik Faye-Lund]

On Tue, Jul 28, 2009 at 1:38 PM, Nicolas Sebrecht<nicolas.s.dev@gmx.fr> wrote:
> Hum, you're right. The '@' symbol is required, whatever "id-right" is.
> My previous patch should fix it.

With all due respect, I don't see how that patch fixes anything. The
previous last-resort solution should already be just as valid, it
assigns 'user@'+hostname to $du_part. Even if hostname is "" it should
insert an '@', which didn't happen here.

I'm suspecting that git-send-email in v1.5.2.5 didn't do enough
checks, and that this is an already-solved issue. Looking at the
source code from v1.5.2.5 seems to confirm this.
http://repo.or.cz/w/git.git?a=blob;f=git-send-email.perl;h=7c0c90bd21bbb009de81aa315bed1c947a32c423;hb=b13ef4916ac5a25cc5897f85ba0b4c5953cff609

my $message_id_from = extract_valid_address($from);
my $message_id_template = "<%s-git-send-email-$message_id_from>";

sub make_message_id
{
	my $date = time;
	my $pseudo_rand = int (rand(4200));
	$message_id = sprintf $message_id_template, "$date$pseudo_rand";
	#print "new message id = $message_id\n"; # Was useful for debugging
}

So I think it's pretty safe to disregard this as an already solved issue.

-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: git-send-email generates mail with invalid Message-Id

[Erik Faye-Lund]

On Tue, Jul 28, 2009 at 1:47 PM, Erik Faye-Lund<kusmabite@googlemail.com> wrote:
> On Tue, Jul 28, 2009 at 1:38 PM, Nicolas Sebrecht<nicolas.s.dev@gmx.fr> wrote:
>> Hum, you're right. The '@' symbol is required, whatever "id-right" is.
>> My previous patch should fix it.
>
> With all due respect, I don't see how that patch fixes anything. The
> previous last-resort solution should already be just as valid, it
> assigns 'user@'+hostname to $du_part. Even if hostname is "" it should
> insert an '@', which didn't happen here.

Here's an attempt to fix the case when Sys::Hostname::hostname returns
"" (domains aren't allowed to be empty if I read RFC2822 correctly).
The problem with the previous attempt was that the earlier if assigned
"user@" to $du_part, so the last if was never entered ($du_part was
always defined).

I generally don't write Perl, so people will most likely barf all over
this one, but at least it should show the concept. It might not even
work.

I also suspect that it is not needed.
http://search.cpan.org/~tty/kurila-1.19_0/ext/Sys-Hostname/Hostname.pm
seems to indicate that it either returns something sensible or dies.

---
Untested.

 git-send-email.perl |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 303e03a..baadbdb 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -742,7 +742,11 @@ sub make_message_id
        }
        if (not defined $du_part or $du_part eq '') {
                use Sys::Hostname qw();
-               $du_part = 'user@' . Sys::Hostname::hostname();
+               my $domain = Sys::Hostname::hostname();
+               if (not defined $domain or $domain eq '') {
+                       $domain = 'fake.dom';
+               }
+               $du_part = "user@$domain";
        }
        my $message_id_template = "<%s-git-send-email-%s>";
        $message_id = sprintf($message_id_template, $uniq, $du_part);
-- 
Erik "kusma" Faye-Lund
kusmabite@gmail.com
(+47) 986 59 656

Re: git-send-email generates mail with invalid Message-Id

[Nicolas Sebrecht]

The 28/07/09, Erik Faye-Lund wrote:
> 
> Here's an attempt to fix the case when Sys::Hostname::hostname returns
> "" (domains aren't allowed to be empty if I read RFC2822 correctly).
>
> The problem with the previous attempt was that the earlier if assigned
> "user@" to $du_part, so the last if was never entered ($du_part was
> always defined).

Yes, thank you.

> I generally don't write Perl, so people will most likely barf all over
> this one, but at least it should show the concept. It might not even
> work.

Looks ok here.

> I also suspect that it is not needed.

I'm not sure because http://linux.die.net/man/2/gethostname does not
tell either (and POSIX neither).

That said, I tend to think it worth to merge this fix before having a
bug report.

-- 
Nicolas Sebrecht