[PATCH 0/4] FreeBSD and test fixes

[PATCH 0/4] FreeBSD and test fixes

[Eric Wong]

Looping the tests for hours on end with a FreeBSD 11.2 VM
revealed some problems (and a minor cleanup) I didn't
find on my Debian GNU/Linux systems.

Eric Wong (4):
  testcommon: DESTROY: wait for killed daemon
  dskqxs: ignore EV_SET errors on EVFILT_WRITE
  t/*.t: localize $SIG{__WARN__} changes
  t/httpd-corner.t: relax read-after-failed-write handling

 lib/PublicInbox/DSKQXS.pm     |  4 ++--
 lib/PublicInbox/TestCommon.pm |  6 +++---
 lib/PublicInbox/Xapcmd.pm     |  6 +++++-
 t/httpd-corner.t              | 39 +++++++++++++++++++++--------------
 t/mda_filter_rubylang.t       |  2 +-
 t/nntpd.t                     |  2 +-
 t/watch_filter_rubylang.t     |  2 +-
 7 files changed, 36 insertions(+), 25 deletions(-)

[PATCH 1/4] testcommon: DESTROY: wait for killed daemon

[Eric Wong]

Otherwise, the waitpid(-1, 0) call in Xapcmd::process_queue()
may reap it in a subsequent test when using t/run.perl to reuse
processes for testing.

While we're at it, make Xapcmd::process_queue warn about unknown
PIDs in case other PIDs leak through to us in the future.
---
 lib/PublicInbox/TestCommon.pm | 6 +++---
 lib/PublicInbox/Xapcmd.pm     | 6 +++++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/TestCommon.pm b/lib/PublicInbox/TestCommon.pm
index 6e3e9d8c..e9efbac7 100644
--- a/lib/PublicInbox/TestCommon.pm
+++ b/lib/PublicInbox/TestCommon.pm
@@ -319,8 +319,9 @@ sub kill {
 }
 
 sub join {
-	my ($self) = @_;
+	my ($self, $sig) = @_;
 	my $pid = delete $self->{pid} or return;
+	CORE::kill($sig, $pid) if defined $sig;
 	my $ret = waitpid($pid, 0);
 	defined($ret) or die "waitpid($pid): $!";
 	$ret == $pid or die "waitpid($pid) != $ret";
@@ -333,8 +334,7 @@ sub DESTROY {
 		PublicInbox::TestCommon::wait_for_tail();
 		CORE::kill('TERM', $tail);
 	}
-	my $pid = delete $self->{pid} or return;
-	CORE::kill('TERM', $pid);
+	$self->join('TERM');
 }
 
 1;
diff --git a/lib/PublicInbox/Xapcmd.pm b/lib/PublicInbox/Xapcmd.pm
index 8e2b9063..337978bd 100644
--- a/lib/PublicInbox/Xapcmd.pm
+++ b/lib/PublicInbox/Xapcmd.pm
@@ -142,7 +142,11 @@ sub process_queue {
 		while (scalar keys %pids) {
 			my $pid = waitpid(-1, 0);
 			my $args = delete $pids{$pid};
-			die join(' ', @$args)." failed: $?\n" if $?;
+			if ($args) {
+				die join(' ', @$args)." failed: $?\n" if $?;
+			} else {
+				warn "unknown PID($pid) reaped: $?\n";
+			}
 		}
 	}
 }

[PATCH 2/4] dskqxs: ignore EV_SET errors on EVFILT_WRITE

[Eric Wong]

Just like the EPOLL_CTL_ADD emulation path, the EPOLL_CTL_MOD
and EPOLL_CTL_DEL emulation paths can fail if attempting to
install an EVFILT_WRITE for a read-only pipe.

I've only observed this on the EPOLL_CTL_DEL emulation path, but
I suspect it could happen on the EPOLL_CTL_MOD path as well.

Increasing the amount of read-only pipes we rely on with altid
exports via sqlite3 made this old bug more apparent and
reproducible while looping the test suite.

This may be adjusted in the future to deal with write-only
pipes, but we currently don't have any of those watched by
kqueue.
---
 lib/PublicInbox/DSKQXS.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/DSKQXS.pm b/lib/PublicInbox/DSKQXS.pm
index b9b0b6c5..35cdecda 100644
--- a/lib/PublicInbox/DSKQXS.pm
+++ b/lib/PublicInbox/DSKQXS.pm
@@ -105,10 +105,10 @@ sub epoll_ctl {
 	my $kq = $self->{kq};
 	if ($op == EPOLL_CTL_MOD) {
 		$kq->EV_SET($fd, EVFILT_READ, kq_flag(EPOLLIN, $ev));
-		$kq->EV_SET($fd, EVFILT_WRITE, kq_flag(EPOLLOUT, $ev));
+		eval { $kq->EV_SET($fd, EVFILT_WRITE, kq_flag(EPOLLOUT, $ev)) };
 	} elsif ($op == EPOLL_CTL_DEL) {
 		$kq->EV_SET($fd, EVFILT_READ, EV_DISABLE);
-		$kq->EV_SET($fd, EVFILT_WRITE, EV_DISABLE);
+		eval { $kq->EV_SET($fd, EVFILT_WRITE, EV_DISABLE) };
 	} else { # EPOLL_CTL_ADD
 		$kq->EV_SET($fd, EVFILT_READ, EV_ADD|kq_flag(EPOLLIN, $ev));
 

[PATCH 3/4] t/*.t: localize $SIG{__WARN__} changes

[Eric Wong]

We don't want to propagate %SIG changes to other tests when
running multiple tests within the same process via t/run.perl.
---
 t/mda_filter_rubylang.t   | 2 +-
 t/nntpd.t                 | 2 +-
 t/watch_filter_rubylang.t | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/mda_filter_rubylang.t b/t/mda_filter_rubylang.t
index dbcb275b..6f288b7e 100644
--- a/t/mda_filter_rubylang.t
+++ b/t/mda_filter_rubylang.t
@@ -18,7 +18,7 @@ is(system(@cfg, 'publicinboxmda.spamcheck', 'none'), 0);
 
 for my $v (qw(V1 V2)) {
 	my @warn;
-	$SIG{__WARN__} = sub { push @warn, @_ };
+	local $SIG{__WARN__} = sub { push @warn, @_ };
 	my $cfgpfx = "publicinbox.$v";
 	my $inboxdir = "$tmpdir/$v";
 	my $addr = "test-$v\@example.com";
diff --git a/t/nntpd.t b/t/nntpd.t
index 43b14d66..826e3f3d 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -268,7 +268,7 @@ Date: Fri, 02 Oct 1993 00:00:00 +0000
 			$for_leafnode->header_set('Message-ID', @mids);
 			$for_leafnode->body_set('not-a-dupe');
 			my $warn = '';
-			$SIG{__WARN__} = sub { $warn .= join('', @_) };
+			local $SIG{__WARN__} = sub { $warn .= join('', @_) };
 			$im->add($for_leafnode);
 			$im->done;
 			like($warn, qr/reused/, 'warned for reused MID');
diff --git a/t/watch_filter_rubylang.t b/t/watch_filter_rubylang.t
index b4540660..09217d94 100644
--- a/t/watch_filter_rubylang.t
+++ b/t/watch_filter_rubylang.t
@@ -24,7 +24,7 @@ SKIP: {
 
 for my $v (@v) {
 	my @warn;
-	$SIG{__WARN__} = sub { push @warn, @_ };
+	local $SIG{__WARN__} = sub { push @warn, @_ };
 	my $cfgpfx = "publicinbox.$v";
 	my $inboxdir = "$tmpdir/$v";
 	my $maildir = "$tmpdir/md-$v";

[PATCH 4/4] t/httpd-corner.t: relax read-after-failed-write handling

[Eric Wong]

I've observed FreeBSD 11.2 read(2) having one of three
behaviors after a failed write(2) on a socket:

1) returning number of bytes read
2) failing with ECONNRESET
3) returning with EOF

1) is the most common, and I've only seen 1) on Linux.  It may
be possible to use SO_LINGER or shutdown(2) to ensure 1) always
happens, but SO_LINGER behavior seems inconsistent across OSes,
especially with non-blocking sockets.

Since these tests are corner-cases where we're dealing with
broken/malicious clients, lets continue spending the least
amount of syscalls protecting ourselves in the daemon and
instead make the client-side test code tolerate more socket
implementations.
---
 t/httpd-corner.t | 39 +++++++++++++++++++++++----------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/t/httpd-corner.t b/t/httpd-corner.t
index 89f2866b..21b5c560 100644
--- a/t/httpd-corner.t
+++ b/t/httpd-corner.t
@@ -122,36 +122,48 @@ if ('test worker death') {
 	is(scalar(grep(/CLOSE FAIL/, @$after)), 1, 'body->close not called');
 }
 
-SKIP: {
+sub check_400 {
+	my ($conn) = @_;
+	my $r = $conn->read(my $buf, 8192);
+	# ECONNRESET and $r==0 are both observed on FreeBSD 11.2
+	if (!defined($r)) {
+		ok($!{ECONNRESET}, 'ECONNRESET on read (BSD sometimes)');
+	} elsif ($r > 0) {
+		like($buf, qr!\AHTTP/1\.\d 400 !, 'got 400 response');
+	} else {
+		is($r, 0, 'got EOF (BSD sometimes)');
+	}
+	close($conn); # ensure we don't get SIGPIPE later
+}
+
+{
+	local $SIG{PIPE} = 'IGNORE';
 	my $conn = conn_for($sock, 'excessive header');
-	$SIG{PIPE} = 'IGNORE';
 	$conn->write("GET /callback HTTP/1.0\r\n");
 	foreach my $i (1..500000) {
 		last unless $conn->write("X-xxxxxJunk-$i: omg\r\n");
 	}
 	ok(!$conn->write("\r\n"), 'broken request');
-	ok($conn->read(my $buf, 8192), 'read response');
-	my ($head, $body) = split(/\r\n\r\n/, $buf);
-	like($head, qr/\b400\b/, 'got 400 response');
+	check_400($conn);
 }
 
 {
 	my $conn = conn_for($sock, 'excessive body Content-Length');
-	$SIG{PIPE} = 'IGNORE';
 	my $n = (10 * 1024 * 1024) + 1;
 	$conn->write("PUT /sha1 HTTP/1.0\r\nContent-Length: $n\r\n\r\n");
-	ok($conn->read(my $buf, 8192), 'read response');
+	my $r = $conn->read(my $buf, 8192);
+	ok($r > 0, 'read response');
 	my ($head, $body) = split(/\r\n\r\n/, $buf);
 	like($head, qr/\b413\b/, 'got 413 response');
 }
 
 {
 	my $conn = conn_for($sock, 'excessive body chunked');
-	$SIG{PIPE} = 'IGNORE';
 	my $n = (10 * 1024 * 1024) + 1;
 	$conn->write("PUT /sha1 HTTP/1.1\r\nTransfer-Encoding: chunked\r\n");
 	$conn->write("\r\n".sprintf("%x\r\n", $n));
-	ok($conn->read(my $buf, 8192), 'read response');
+	my $r = $conn->read(my $buf, 8192);
+	ok($r > 0, 'read response');
 	my ($head, $body) = split(/\r\n\r\n/, $buf);
 	like($head, qr/\b413\b/, 'got 413 response');
 }
@@ -410,10 +422,7 @@ SKIP: {
 	ok($!, 'got error set in $!');
 	is($w, undef, 'write error happened');
 	ok($n > 0, 'was able to write');
-	my $r = $conn->read(my $buf, 66666);
-	ok($r > 0, 'got non-empty response');
-	like($buf, qr!HTTP/1\.\d 400 !, 'got 400 response');
-
+	check_400($conn);
 	$conn = conn_for($sock, '1.1 chunk trailer excessive');
 	$conn->write("PUT /sha1 HTTP/1.1\r\nTransfer-Encoding:chunked\r\n\r\n");
 	is($conn->syswrite("1\r\na"), 4, 'wrote first header + chunk');
@@ -424,9 +433,7 @@ SKIP: {
 	}
 	ok($!, 'got error set in $!');
 	ok($n > 0, 'wrote part of chunk end (\r)');
-	$r = $conn->read($buf, 66666);
-	ok($r > 0, 'got non-empty response');
-	like($buf, qr!HTTP/1\.\d 400 !, 'got 400 response');
+	check_400($conn);
 }
 
 {