Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address"

Hello,

	$ git send-email --to 'A B <a@b.org>, C D <c@d.org>' lala.patch
	Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172.
	error: unable to extract a valid address from:

This happens for me with git 2.42.0 and also on master (59167d7d09fd, "The seventeenth batch").

Bisection points at

	a8022c5f7b67 ("send-email: expose header information to git-send-email's sendemail-validate hook")

I didn't try to understand that change and fix the problem.

Best regards Uwe

On Fri, Oct 13, 2023, at 16:14, Uwe Kleine-König wrote:

This looks the same problem as https://lore.kernel.org/git/ZQ1eGzqfyoeeTBUq@debian.me/

hello,

On Fri, Oct 13, 2023 at 04:14:37PM +0200, Uwe Kleine-König wrote:

Another (similar?) problem with non-ascii-chars:

	$ git send-email --to 'Will Deacon <will@kernel.org>' --to 'Krzysztof Wilczyński <kw@linux.com>' --to 'Lorenzo Pieralisi <lpieralisi@kernel.org>' --cc 'Rob Herring <robh@kernel.org>' --to 'Bjorn Helgaas <bhelgaas@google.com>' --cc 'linux-pci@vger.kernel.org' --cc kernel@pengutronix.de -1 --base=@~
	Use of uninitialized value $address in sprintf at /home/uwe/gsrc/git/git-send-email line 1162.
	error: unable to extract a valid address from:

Bisection points to the same commit, when dropping ń in Krzysztof's name, it works fine.

Best regards Uwe

On 10/20/23 06:04, Uwe Kleine-König wrote:

This is interesting.  Thanks for reporting it.  If you are able, could you please try the patches found in the below threads:
- https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#mae64003cbb72f015bf5c0c04216524fcb6bb8d09
- https://public-inbox.org/git/f5c6a72b-f888-4d43-8be8-3ce2c878c669@gmail.com/T/#mca12dc95ccfd3ce2b94e7752ebaae9891201084f

Thanks, Michael

Hello,

On Fri, Oct 20, 2023 at 05:06:36PM -0400, Michael Strawbridge wrote:

On main (2e8e77cbac8a) this one is already applied, with that the error message reduces to:

	$ git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' -1
	error: unable to extract a valid address from: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

This one doesn't help either. With it applied on top of main I get the sams result as on vanilla main.

Best regards Uwe

On 10/24/23 09:00, Uwe Kleine-König wrote:

Hm. I tried reproing with master (59167d7d09fd, "The seventeenth batch") but I don't seem to see an error: ``` $ git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' -1 --smtp-server="$(pwd)/fake.sendmail"

/tmp/FEGzLrm_4U/0001-test-test.patch 1 (mbox) Adding cc: Your Name <you@example.com> from line 'From: Your Name <you@example.com>' (body) Adding cc: Your Name <you@example.com> from line 'Signed-off-by: Your Name <you@example.com>'

From: Your Name <you@example.com>
To: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc: Your Name <you@example.com>
Subject: [PATCH] test test
Date: Tue, 24 Oct 2023 18:33:51 +0000
Message-ID: <20231024183350.10-2-you@example.com>
X-Mailer: git-send-email 2.40.0.rc1.1708.gbffa95ed23.dirty
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

    The Cc list above has been expanded by additional
    addresses found in the patch commit message. By default
    send-email prompts before sending whenever this occurs.
    This behavior is controlled by the sendemail.confirm
    configuration setting.

    For additional information, run 'git send-email --help'.
    To retain the current behavior, but squelch this message,
    run 'git config --global sendemail.confirm auto'.

Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
-i
OK. Log says:
Sendmail: /git/fake.sendmail -i u.kleine-koenig@pengutronix.de you@example.com
From: Your Name <you@example.com>
To: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc: Your Name <you@example.com>
Subject: [PATCH] test test
Date: Tue, 24 Oct 2023 18:33:51 +0000
Message-ID: <20231024183350.10-2-you@example.com>
X-Mailer: git-send-email 2.40.0.rc1.1708.gbffa95ed23.dirty
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

Result: OK
```

I think my terminal may be mangling the UTF-8 encoding but git send-email doesn't seem to complain.

Only when I go to an older git (version 2.42.0 for example) do I then get:

``` Use of uninitialized value $address in sprintf at /usr/lib/git-core/git-send-email line 1172. error: unable to extract a valid address from: ```

This would be expected because https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#mae64003cbb72f015bf5c0c04216524fcb6bb8d09 is meant to fix it.

Just in case it was the "from" address, I tried using 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' as the from address. This still worked without issue for (59167d7d09fd, "The seventeenth batch").

I tried this as well with seventeen and it succeeds: ``` /tmp/GfTxbwKSx_/0001-test-test.patch 1 (mbox) Adding cc: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de> from line 'From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>' (body) Adding cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> from line 'Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>'

From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
To: Will Deacon <will@kernel.org>,
	=?UTF-8?q?Krzysztof=20Wilczy=C5=84ski?= <kw@linux.com>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>,
	linux-pci@vger.kernel.org,
	kernel@pengutronix.de,
	=?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Subject: [PATCH] test test
Date: Tue, 24 Oct 2023 18:57:16 +0000
Message-ID: <20231024185715.10-2-u.kleine-koenig@pengutronix.de>
X-Mailer: git-send-email 2.40.0.rc1.1708.gbffa95ed23.dirty
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

    The Cc list above has been expanded by additional
    addresses found in the patch commit message. By default
    send-email prompts before sending whenever this occurs.
    This behavior is controlled by the sendemail.confirm
    configuration setting.

    For additional information, run 'git send-email --help'.
    To retain the current behavior, but squelch this message,
    run 'git config --global sendemail.confirm auto'.

Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): y
-i
OK. Log says:
Sendmail: /git/fake.sendmail -i will@kernel.org kw@linux.com lpieralisi@kernel.org bhelgaas@google.com robh@kernel.org linux-pci@vger.kernel.org kernel@pengutronix.de u.kleine-koenig@pengutronix.de
From: =?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
To: Will Deacon <will@kernel.org>,
	=?UTF-8?q?Krzysztof=20Wilczy=C5=84ski?= <kw@linux.com>,
	Lorenzo Pieralisi <lpieralisi@kernel.org>,
	Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>,
	linux-pci@vger.kernel.org,
	kernel@pengutronix.de,
	=?UTF-8?q?Uwe=20Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Subject: [PATCH] test test
Date: Tue, 24 Oct 2023 18:57:16 +0000
Message-ID: <20231024185715.10-2-u.kleine-koenig@pengutronix.de>
X-Mailer: git-send-email 2.40.0.rc1.1708.gbffa95ed23.dirty
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Result: OK
```

Perhaps there is another variable involved?

Hello Michael,

On Tue, Oct 24, 2023 at 03:00:38PM -0400, Michael Strawbridge wrote:

I debugged a bit and if I do

	mv .git/hooks/sendemail-validate .git/hooks/sendemail-validate.bak

git send-email --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>' starts to work for me, too.

I'd guess the content of my sendemail-validate script doesn't matter much, but for the record, it's:

	#!/bin/sh
	# installed by patatt install-hook
	patatt sign --hook "${1}"

Does the problem reproduce on your end with a sendemail-validate script?

Best regards Uwe

On Tue, Oct 24, 2023 at 10:43:18PM +0200, Uwe Kleine-König wrote:

I can reproduce with:

  git init
  echo foo >file && git add file && git commit -m foo
  echo 'exit 0' >.git/hooks/sendemail-validate
  chmod +x .git/hooks/sendemail-validate
  git send-email --dry-run --to='pëff <peff@peff.net>' -1

Note that the bug will only trigger if Email::Valid is installed. I think this is the same issue being discussed elsewhere. The call to process_address_list() sanitizes it to use rfc2047 encoding, which is necessary for it to be syntactically valid.

So the patch to move the validation later in the process here:

  https://lore.kernel.org/git/ee56c4df-e030-45f9-86a9-94fb3540db60@amd.com/

fixes it.

-Peff

Hello,

On Wed, Oct 25, 2023 at 03:21:04AM -0400, Jeff King wrote:

I can confirm I have this package installed (via Debian's libemail-valid-perl).

Tested and indeed that fixes my usecase. With that patch also the original regression is fixed and I can do

	git send-email -1 --to 'Uwe Kleine-König <u.kleine-koenig@pengutronix.de>, pëff <peff@peff.net>'

again. \o/

Thanks Uwe

Jeff King <peff@peff.net> writes:

I recall we chased a different bug that depends on the use/non-use of this package a few years ago. Is the difference significant enough that we may want to install on one but not in another CI environment, like we have a separate CI jobs with exotic settings, I wonder.

On 10/26/23 08:41, Junio C Hamano wrote:

That would make sense to me. We have had 3 regressions threads recently for git send email where Email::Valid was important.

- [REGRESSION] uninitialized value $address in git send-email when given multiple recipients separated by commas - (this thread)
- [REGRESSION] uninitialized value $address in git send-email - https://public-inbox.org/git/20230918212004.GC2163162@coredump.intra.peff.net/T/#m9e0211a8ad387adbbadf31dcfcd7982d4046633d
- Regression: git send-email fails with "Use of uninitialized value $address" + "unable to extract a valid address" - https://public-inbox.org/git/68d7e5c3-6b4a-4d0d-9885-f3d4e2199f26@amd.com/T/#m1411c155e11ad9c5d913d22d1d11180ed56eabc7

Michael Strawbridge wrote:

Alternately, perhaps having Email::Valid as an optional dependency is worth reconsidering. If it's truly important to validation, make it a requirement. If it's not, then drop it to simplify the code and avoid these sort of issues.

As a (former) distribution packager, having these optional dependencies which change the behavior is always a tough position to be in.

If I make the git package require it to ensure consistent behavior then some folks will -quite rightly- complain that it should not be a requirement. If I keep it an optional dependency, then debugging becomes more difficult for the reasons we've seen in these recent (and not-so-recent) threads.

I'd lean toward dropping the dependency entirely and leave the more basic validation of git-send-email in place. That may not catch every type of address error, but I would argue that what we do without Email::Valid is perfectly reasonable for checking basic email address syntax sanity.

Further validation will happen along the path of mail transfer agents and failures should be reported to the sender in the same way as any other invalid email address.

On a related note, one issue¹ we had reported in Fedora after making Email::Valid a requirement was that it rejected messages where the local part was too long, per the relevant RFC's. But these were generated addresses from GitLab. The addresses worked in practice. While Email::Valid was technically correct in rejecting such addresses, it didn't improve the experience of git send-email users.

¹ https://bugzilla.redhat.com/2046203

Todd Zullinger <tmz@pobox.com> writes:

Reducing the possible "valid" configurations we support is a very tempting proposition.

Very true.

Yes, it is very tempting, and given that we have to keep our fallback codepath working for those without Email::Valid ANYWAY, as long as the dependency is merely optional, I very much agree with your argument here.

I am of two minds here.  I can sympathize with both positions.

 - Trying to be strict to what we send out to the world by using
   Email::Valid that tries to be more RFC kosher matches "be strict
   in what you send out, be lenient in what you receive" mantra

 - Rejecting what works in practice and in real world tend to help
   users.

If we require Email::Valid, then sriking the balance between the above two will entirely become the responsibility of them; any end-user who complains "the validation is overly strict" will get "talk to authors of Email::Valid".

If we ditch Email::Valid, it will become _our_ responsibility, which means a bit of extra maintenance burden to this project. But perhaps it is worth it? I dunno.

Having Email::Valid as an optional dependency does not place us in a position better than either of these two, so from that point of view, too, I like your "we should either make it required or unused, not an optional dependency" very much.

Thanks.

On Thu, Oct 26, 2023 at 10:46:30AM -0400, Todd Zullinger wrote:

I am somewhat tempted to say the same, but in this case didn't it help us find a real bug? True, the bug was that we were feeding garbage to Email::Valid because we were calling it to early, and I _think_ the ultimate emails we sent would have been OK. But I think we were possibly feeding that garbage to the user-visible validation hook.

That might just mean we need to beef up our homegrown validation a bit (though as you indicate here, I am generally of the opinion that the best validation is trying to actually send things).

-Peff