unofficial mirror of meta@public-inbox.org
 help / color / mirror / Atom feed
* [PATCH 0/9] lei: various corner case leak fixes
@ 2021-03-24  9:23 Eric Wong
  2021-03-24  9:23 ` [PATCH 1/9] ds: improve DS->Reset fork-safety Eric Wong
                   ` (8 more replies)
  0 siblings, 9 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

Making the test suite use a single lei-daemon for all tests has
uncovered a number of bugs that wouldn't get uncovered during
normal usage.  These fixes could be useful if lei-daemon is
deployed a multi-tenant server with both shared and "private"
storage support running under a single Unix user.

Eric Wong (9):
  ds: improve DS->Reset fork-safety
  mbox_lock: dotlock: chdir for relative lock paths
  lei: drop circular reference in lei_store process
  lei: update {3} after -C chdirs
  lei: clean up pkt_op consumer on exception, too
  lei_store: give process a better name
  v2writable: cleanup SQLite handles on --xapian-only
  lei_mirror: fix circular reference
  lei-daemon: do not leak FDs on bogus requests

 lib/PublicInbox/DS.pm         | 76 +++++++++++++++++++++--------------
 lib/PublicInbox/LEI.pm        | 45 ++++++++++++++++-----
 lib/PublicInbox/LeiMirror.pm  |  2 +-
 lib/PublicInbox/LeiStore.pm   |  6 ++-
 lib/PublicInbox/LeiXSearch.pm |  9 +----
 lib/PublicInbox/MboxLock.pm   | 14 +++++++
 lib/PublicInbox/V2Writable.pm |  1 +
 t/lei-daemon.t                | 29 +++++++++++++
 t/mbox_lock.t                 | 12 ++++++
 t/v2reindex.t                 | 10 ++++-
 10 files changed, 153 insertions(+), 51 deletions(-)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/9] ds: improve DS->Reset fork-safety
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  2021-03-24 23:01   ` [SQUASH] " Eric Wong
  2021-03-24  9:23 ` [PATCH 2/9] mbox_lock: dotlock: chdir for relative lock paths Eric Wong
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

None of these fixes affect current public-inbox-* code, or even
normal uses of lei.  However, lei users wanting to switch
between $HOME directories or use alternate store paths may
notice strange behavior and this fixes some of it.

We'll also loop to account for DESTROY callbacks inserting into
container objects and retry appropriately.
---
 lib/PublicInbox/DS.pm | 76 ++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index 15ece4df..c881c895 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -35,9 +35,10 @@ use Errno qw(EAGAIN EINVAL);
 use Carp qw(carp croak);
 our @EXPORT_OK = qw(now msg_more dwaitpid add_timer);
 
+our %Stack;
 my $nextq; # queue for next_tick
 my $wait_pids; # list of [ pid, callback, callback_arg ]
-my $later_queue; # list of callbacks to run at some later interval
+my $later_q; # list of callbacks to run at some later interval
 my $EXPMAP; # fd -> idle_time
 our $EXPTIME = 180; # 3 minutes
 my ($later_timer, $reap_armed, $exp_timer);
@@ -65,19 +66,27 @@ Reset();
 Reset all state
 
 =cut
+our %state;
 sub Reset {
-    $in_loop = undef; # first in case DESTROY callbacks use this
-    %DescriptorMap = ();
-    $wait_pids = $later_queue = $reap_armed = undef;
-    $EXPMAP = {};
-    $nextq = $ToClose = $later_timer = $exp_timer = undef;
-    $LoopTimeout = -1;  # no timeout by default
-    @Timers = ();
-
-    $PostLoopCallback = undef;
-
-    $_io = undef; # closes real $Epoll FD
-    $Epoll = undef; # may call DSKQXS::DESTROY
+	do {
+		$in_loop = undef; # first in case DESTROY callbacks use this
+		%DescriptorMap = ();
+		@Timers = ();
+		$PostLoopCallback = undef;
+
+		# we may be iterating inside one of these on our stack
+		my @q = delete @Stack{keys %Stack};
+		for my $q (@q) { @$q = () }
+		$EXPMAP = {};
+		$wait_pids = $later_q = $nextq = $ToClose = undef;
+		$_io = undef; # closes real $Epoll FD
+		$Epoll = undef; # may call DSKQXS::DESTROY
+	} while (@Timers || keys(%Stack) || $nextq || $wait_pids ||
+		$later_q || $ToClose || keys(%DescriptorMap) ||
+		$PostLoopCallback);
+
+	$reap_armed = $later_timer = $exp_timer = undef;
+	$LoopTimeout = -1;  # no timeout by default
 }
 
 =head2 C<< CLASS->SetLoopTimeout( $timeout ) >>
@@ -160,17 +169,19 @@ C<PostLoopCallback> below for how to exit the loop.
 sub now () { clock_gettime(CLOCK_MONOTONIC) }
 
 sub next_tick () {
-    my $q = $nextq or return;
-    $nextq = undef;
-    for my $obj (@$q) {
-        # we avoid "ref" on blessed refs to workaround a Perl 5.16.3 leak:
-        # https://rt.perl.org/Public/Bug/Display.html?id=114340
-        if (blessed($obj)) {
-            $obj->event_step;
-        } else {
-            $obj->();
-        }
-    }
+	my $q = $nextq or return;
+	$nextq = undef;
+	$Stack{cur_runq} = $q;
+	for my $obj (@$q) {
+		# avoid "ref" on blessed refs to workaround a Perl 5.16.3 leak:
+		# https://rt.perl.org/Public/Bug/Display.html?id=114340
+		if (blessed($obj)) {
+			$obj->event_step;
+		} else {
+			$obj->();
+		}
+	}
+	delete $Stack{cur_runq};
 }
 
 # runs timers and returns milliseconds for next one, or next event loop
@@ -221,6 +232,7 @@ sub reap_pids {
 	$reap_armed = undef;
 	my $tmp = $wait_pids or return;
 	$wait_pids = undef;
+	$Stack{reap_runq} = $tmp;
 	my $oldset = block_signals();
 	foreach my $ary (@$tmp) {
 		my ($pid, $cb, $arg) = @$ary;
@@ -237,6 +249,7 @@ sub reap_pids {
 		}
 	}
 	sig_setmask($oldset);
+	delete $Stack{reap_runq};
 }
 
 # reentrant SIGCHLD handler (since reap_pids is not reentrant)
@@ -271,7 +284,6 @@ sub EventLoop {
     $Epoll //= _InitPoller();
     local $in_loop = 1;
     my @events;
-    my $obj; # guard stack-not-refcounted w/ Carp + @DB::args
     do {
         my $timeout = RunTimers();
 
@@ -282,7 +294,9 @@ sub EventLoop {
             # that ones in the front triggered unregister-interest actions.  if we
             # can't find the %sock entry, it's because we're no longer interested
             # in that event.
-            $obj = $DescriptorMap{$fd};
+
+	    # guard stack-not-refcounted w/ Carp + @DB::args
+            my $obj = $DescriptorMap{$fd};
             $obj->event_step;
         }
     } while (PostEventLoop());
@@ -646,13 +660,15 @@ sub dwaitpid ($;$$) {
 }
 
 sub _run_later () {
-	my $run = $later_queue or return;
-	$later_timer = $later_queue = undef;
-	$_->() for @$run;
+	my $q = $later_q or return;
+	$later_timer = $later_q = undef;
+	$Stack{later_q} = $q;
+	$_->() for @$q;
+	delete $Stack{later_q};
 }
 
 sub later ($) {
-	push @$later_queue, $_[0]; # autovivifies @$later_queue
+	push @$later_q, $_[0]; # autovivifies @$later_q
 	$later_timer //= add_timer(60, \&_run_later);
 }
 

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/9] mbox_lock: dotlock: chdir for relative lock paths
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
  2021-03-24  9:23 ` [PATCH 1/9] ds: improve DS->Reset fork-safety Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  2021-03-24  9:23 ` [PATCH 3/9] lei: drop circular reference in lei_store process Eric Wong
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

Since lei-daemon will fchdir on every request, we must ensure
we're in the correct directory before unlink(2) is called,
since we can't use unlinkat(2) from pure Perl.
---
 lib/PublicInbox/MboxLock.pm | 14 ++++++++++++++
 t/mbox_lock.t               | 12 ++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/lib/PublicInbox/MboxLock.pm b/lib/PublicInbox/MboxLock.pm
index bea0e325..856b1e21 100644
--- a/lib/PublicInbox/MboxLock.pm
+++ b/lib/PublicInbox/MboxLock.pm
@@ -60,6 +60,11 @@ sub acq_dotlock {
 			if (link($tmp, $dot_lock)) {
 				unlink($tmp) or die "unlink($tmp): $!";
 				$self->{".lock$pid"} = $dot_lock;
+				if (substr($dot_lock, 0, 1) ne '/') {
+					opendir(my $dh, '.') or
+							die "opendir . $!";
+					$self->{dh} = $dh;
+				}
 				return;
 			}
 			unlink($tmp) or die "unlink($tmp): $!";
@@ -111,10 +116,19 @@ sub acq {
 	$self;
 }
 
+sub _fchdir { chdir($_[0]) } # OnDestroy callback
+
 sub DESTROY {
 	my ($self) = @_;
 	if (my $f = $self->{".lock$$"}) {
+		my $x;
+		if (my $dh = delete $self->{dh}) {
+			opendir my $c, '.' or die "opendir . $!";
+			$x = PublicInbox::OnDestroy->new(\&_fchdir, $c);
+			chdir($dh) or die "chdir (for $f): $!";
+		}
 		unlink($f) or die "unlink($f): $! (lock stolen?)";
+		undef $x;
 	}
 }
 
diff --git a/t/mbox_lock.t b/t/mbox_lock.t
index 3dc3b449..c2fee0d4 100644
--- a/t/mbox_lock.t
+++ b/t/mbox_lock.t
@@ -5,6 +5,7 @@ use strict; use v5.10.1; use PublicInbox::TestCommon;
 use POSIX qw(_exit);
 use PublicInbox::DS qw(now);
 use Errno qw(EAGAIN);
+use PublicInbox::OnDestroy;
 use_ok 'PublicInbox::MboxLock';
 my ($tmpdir, $for_destroy) = tmpdir();
 my $f = "$tmpdir/f";
@@ -15,6 +16,17 @@ ok(!-f "$f.lock", 'dotlock gone');
 $mbl = PublicInbox::MboxLock->acq($f, 1, ['none']);
 ok(!-f "$f.lock", 'no dotlock with none');
 undef $mbl;
+{
+	opendir my $cur, '.' or BAIL_OUT $!;
+	my $od = PublicInbox::OnDestroy->new(sub { chdir $cur });
+	chdir $tmpdir or BAIL_OUT;
+	my $abs = "$tmpdir/rel.lock";
+	my $rel = PublicInbox::MboxLock->acq('rel', 1, ['dotlock']);
+	chdir '/' or BAIL_OUT;
+	ok(-f $abs, 'lock with abs path created');
+	undef $rel;
+	ok(!-f $abs, 'lock gone despite being in the wrong dir');
+}
 
 eval {
 	PublicInbox::MboxLock->acq($f, 1, ['bogus']);

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/9] lei: drop circular reference in lei_store process
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
  2021-03-24  9:23 ` [PATCH 1/9] ds: improve DS->Reset fork-safety Eric Wong
  2021-03-24  9:23 ` [PATCH 2/9] mbox_lock: dotlock: chdir for relative lock paths Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  2021-03-24  9:23 ` [PATCH 4/9] lei: update {3} after -C chdirs Eric Wong
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

I'm not sure if this was causing real problems, but it's sure ugly.
---
 lib/PublicInbox/LEI.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 8cbaac01..ee991f80 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -458,6 +458,9 @@ sub _lei_atfork_child {
 		unless ($self->{oneshot}) {
 			close($_) for @io;
 		}
+		if (my $cfg = $self->{cfg}) {
+			delete $cfg->{-lei_store};
+		}
 	} else { # worker, Net::NNTP (Net::Cmd) uses STDERR directly
 		open STDERR, '+>&='.fileno($self->{2}) or warn "open $!";
 		delete $self->{0};

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/9] lei: update {3} after -C chdirs
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
                   ` (2 preceding siblings ...)
  2021-03-24  9:23 ` [PATCH 3/9] lei: drop circular reference in lei_store process Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  2021-03-24  9:23 ` [PATCH 5/9] lei: clean up pkt_op consumer on exception, too Eric Wong
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

This is necessary for lei->rel2abs correctness, and may
eventually be useful if we can use *at syscalls.
---
 lib/PublicInbox/LEI.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index ee991f80..74372532 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -640,6 +640,11 @@ sub dispatch {
 				next if $d eq ''; # same as git(1)
 				chdir $d or return fail($self, "cd $d: $!");
 			}
+			if (delete $self->{3}) { # update cwd for rel2abs
+				opendir my $dh, '.' or
+					return fail($self, "opendir . $!");
+				$self->{3} = $dh;
+			}
 		}
 		$cb->($self, @argv);
 	} elsif (grep(/\A-/, $cmd, @argv)) { # --help or -h only

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/9] lei: clean up pkt_op consumer on exception, too
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
                   ` (3 preceding siblings ...)
  2021-03-24  9:23 ` [PATCH 4/9] lei: update {3} after -C chdirs Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  2021-03-24  9:23 ` [PATCH 6/9] lei_store: give process a better name Eric Wong
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

We need to consistently ensure pkt_op_c doesn't lead to a
long-lived circular reference if an exception is thrown in
pre_augment.  Maybe the API could be better, but this fixes an
FD leak when attempting to --augment a FIFO.

Followup-to: b9524082ba39e665 ("lei_xsearch: cleanup {pkt_op_p} on exceptions")
---
 lib/PublicInbox/LEI.pm        | 22 ++++++++++++++++++++--
 lib/PublicInbox/LeiXSearch.pm |  9 ++-------
 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 74372532..878685f1 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -482,6 +482,24 @@ sub _lei_atfork_child {
 	$current_lei = $persist ? undef : $self; # for SIG{__WARN__}
 }
 
+sub _delete_pkt_op { # OnDestroy callback to prevent leaks on die
+	my ($self) = @_;
+	if (my $op = delete $self->{pkt_op_c}) { # in case of die
+		$op->close; # PublicInbox::PktOp::close
+	}
+	my $unclosed_after_die = delete($self->{pkt_op_p}) or return;
+	close $unclosed_after_die;
+}
+
+sub pkt_op_pair {
+	my ($self, $ops) = @_;
+	require PublicInbox::OnDestroy;
+	require PublicInbox::PktOp;
+	my $end = PublicInbox::OnDestroy->new($$, \&_delete_pkt_op, $self);
+	@$self{qw(pkt_op_c pkt_op_p)} = PublicInbox::PktOp->pair($ops);
+	$end;
+}
+
 sub workers_start {
 	my ($lei, $wq, $ident, $jobs, $ops) = @_;
 	$ops = {
@@ -492,11 +510,11 @@ sub workers_start {
 		($ops ? %$ops : ()),
 	};
 	$ops->{''} //= [ \&dclose, $lei ];
-	require PublicInbox::PktOp;
-	($lei->{pkt_op_c}, $lei->{pkt_op_p}) = PublicInbox::PktOp->pair($ops);
+	my $end = $lei->pkt_op_pair($ops);
 	$wq->wq_workers_start($ident, $jobs, $lei->oldset, { lei => $lei });
 	delete $lei->{pkt_op_p};
 	my $op = delete $lei->{pkt_op_c};
+	@$end = ();
 	$lei->event_step_init;
 	# oneshot needs $op, daemon-mode uses DS->EventLoop to handle $op
 	$lei->{oneshot} ? $op : undef;
diff --git a/lib/PublicInbox/LeiXSearch.pm b/lib/PublicInbox/LeiXSearch.pm
index c6b82eeb..58b6cfc0 100644
--- a/lib/PublicInbox/LeiXSearch.pm
+++ b/lib/PublicInbox/LeiXSearch.pm
@@ -406,11 +406,6 @@ sub ipc_atfork_child {
 	$self->SUPER::ipc_atfork_child;
 }
 
-sub delete_pkt_op { # OnDestroy callback
-	my $unclosed_after_die = delete($_[0])->{pkt_op_p} or return;
-	close $unclosed_after_die;
-}
-
 sub do_query {
 	my ($self, $lei) = @_;
 	my $l2m = $lei->{l2m};
@@ -426,8 +421,7 @@ sub do_query {
 		'incr_start_query' => [ \&incr_start_query, $self, $l2m ],
 	};
 	$lei->{auth}->op_merge($ops, $l2m) if $l2m && $lei->{auth};
-	my $od = PublicInbox::OnDestroy->new($$, \&delete_pkt_op, $lei);
-	($lei->{pkt_op_c}, $lei->{pkt_op_p}) = PublicInbox::PktOp->pair($ops);
+	my $end = $lei->pkt_op_pair($ops);
 	$lei->{1}->autoflush(1);
 	$lei->start_pager if delete $lei->{need_pager};
 	$lei->{ovv}->ovv_begin($lei);
@@ -446,6 +440,7 @@ sub do_query {
 				$lei->oldset, { lei => $lei });
 	my $op = delete $lei->{pkt_op_c};
 	delete $lei->{pkt_op_p};
+	@$end = ();
 	$self->{threads} = $lei->{opt}->{threads};
 	if ($l2m) {
 		$l2m->net_merge_complete unless $lei->{auth};

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6/9] lei_store: give process a better name
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
                   ` (4 preceding siblings ...)
  2021-03-24  9:23 ` [PATCH 5/9] lei: clean up pkt_op consumer on exception, too Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  2021-03-24  9:23 ` [PATCH 7/9] v2writable: cleanup SQLite handles on --xapian-only Eric Wong
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

We'll prioritize the last two components of the path name
("lei/store") since that's how I often refer to the on-disk
location.  Then, show the XDG_DATA_HOME it belongs to in case
a user changes HOME or XDG_* for testing purposes.
---
 lib/PublicInbox/LeiStore.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/LeiStore.pm b/lib/PublicInbox/LeiStore.pm
index fa03f93c..1311ad46 100644
--- a/lib/PublicInbox/LeiStore.pm
+++ b/lib/PublicInbox/LeiStore.pm
@@ -329,11 +329,13 @@ sub ipc_atfork_child {
 sub write_prepare {
 	my ($self, $lei) = @_;
 	unless ($self->{-ipc_req}) {
-		$self->ipc_lock_init($lei->store_path . '/ipc.lock');
+		my $d = $lei->store_path;
+		$self->ipc_lock_init("$d/ipc.lock");
+		substr($d, -length('/lei/store'), 10, '');
 		# Mail we import into lei are private, so headers filtered out
 		# by -mda for public mail are not appropriate
 		local @PublicInbox::MDA::BAD_HEADERS = ();
-		$self->ipc_worker_spawn('lei_store', $lei->oldset,
+		$self->ipc_worker_spawn("lei/store $d", $lei->oldset,
 					{ lei => $lei });
 	}
 	$lei->{sto} = $self;

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 7/9] v2writable: cleanup SQLite handles on --xapian-only
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
                   ` (5 preceding siblings ...)
  2021-03-24  9:23 ` [PATCH 6/9] lei_store: give process a better name Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  2021-03-24  9:23 ` [PATCH 8/9] lei_mirror: fix circular reference Eric Wong
  2021-03-24  9:23 ` [PATCH 9/9] lei-daemon: do not leak FDs on bogus requests Eric Wong
  8 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

I'm not sure exactly why this is needed with run_script
localizing %SIG and everything else, but explictly cleaning up
seems to fix the occasional test failures I see.

Followup-to: 4c6c853494b49368 ("tests: show lsof output on deleted-file-check failures")
---
 lib/PublicInbox/V2Writable.pm |  1 +
 t/v2reindex.t                 | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/PublicInbox/V2Writable.pm b/lib/PublicInbox/V2Writable.pm
index 03590850..4130a472 100644
--- a/lib/PublicInbox/V2Writable.pm
+++ b/lib/PublicInbox/V2Writable.pm
@@ -1323,6 +1323,7 @@ sub xapian_only {
 		}
 	}
 	$self->git->cat_async_wait;
+	$self->{ibx}->cleanup;
 	$self->done;
 }
 
diff --git a/t/v2reindex.t b/t/v2reindex.t
index 1145e31b..e9f2b73b 100644
--- a/t/v2reindex.t
+++ b/t/v2reindex.t
@@ -549,5 +549,13 @@ my $env = { PI_CONFIG => '/dev/null' };
 ok(run_script([qw(-index --reindex --xapian-only), $inboxdir], $env, $rdr),
 	'--xapian-only works');
 is($err, '', 'no errors from --xapian-only');
-
+undef $for_destroy;
+SKIP: {
+	use PublicInbox::Spawn qw(which);
+	skip 'only testing lsof(8) output on Linux', 1 if $^O ne 'linux';
+	my $lsof = which('lsof') or skip 'no lsof in PATH', 1;
+	my $rdr = { 2 => \(my $null_err) };
+	my @d = grep(m!/xap[0-9]+/!, xqx([$lsof, '-p', $$], undef, $rdr));
+	is_deeply(\@d, [], 'no deleted index files') or diag explain(\@d);
+}
 done_testing();

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 8/9] lei_mirror: fix circular reference
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
                   ` (6 preceding siblings ...)
  2021-03-24  9:23 ` [PATCH 7/9] v2writable: cleanup SQLite handles on --xapian-only Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  2021-03-24  9:23 ` [PATCH 9/9] lei-daemon: do not leak FDs on bogus requests Eric Wong
  8 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

All of our $lei->workers_start callers can simply rely on
that wrapper to do the right thing and pass fields to
->wq_worker_start children, only.

This could manifest as a unbound memory growth if somebody is
constantly mirroring, and was causing tests to get stuck when
experimenting with a persistent lei-daemon for the entire
test suite.
---
 lib/PublicInbox/LeiMirror.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index 6e62625d..d68cd6c1 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -268,7 +268,7 @@ sub do_mirror { # via wq_io_do
 
 sub start {
 	my ($cls, $lei, $src, $dst) = @_;
-	my $self = bless { lei => $lei, src => $src, dst => $dst }, $cls;
+	my $self = bless { src => $src, dst => $dst }, $cls;
 	if ($src =~ m!https?://!) {
 		require URI;
 		require PublicInbox::LeiCurl;

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 9/9] lei-daemon: do not leak FDs on bogus requests
  2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
                   ` (7 preceding siblings ...)
  2021-03-24  9:23 ` [PATCH 8/9] lei_mirror: fix circular reference Eric Wong
@ 2021-03-24  9:23 ` Eric Wong
  8 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24  9:23 UTC (permalink / raw)
  To: meta

If a client passes us the incorrect number of FDs, we'll vivify
them into PerlIO objects so they can be auto-closed.  Using
POSIX::close was considered, but it would've been more code to
handle an uncommon case.
---
 lib/PublicInbox/LEI.pm | 15 +++++++--------
 t/lei-daemon.t         | 29 +++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/lib/PublicInbox/LEI.pm b/lib/PublicInbox/LEI.pm
index 878685f1..e5211764 100644
--- a/lib/PublicInbox/LEI.pm
+++ b/lib/PublicInbox/LEI.pm
@@ -981,17 +981,16 @@ sub accept_dispatch { # Listener {post_accept} callback
 		return send($sock, 'timed out waiting to recv FDs', MSG_EOR);
 	# (4096 * 33) >MAX_ARG_STRLEN
 	my @fds = $recv_cmd->($sock, my $buf, 4096 * 33) or return; # EOF
-	if (scalar(@fds) == 4) {
-		for my $i (0..3) {
-			my $fd = shift(@fds);
-			open($self->{$i}, '+<&=', $fd) and next;
-			send($sock, "open(+<&=$fd) (FD=$i): $!", MSG_EOR);
-		}
-	} elsif (!defined($fds[0])) {
+	if (!defined($fds[0])) {
 		warn(my $msg = "recv_cmd failed: $!");
 		return send($sock, $msg, MSG_EOR);
 	} else {
-		return;
+		my $i = 0;
+		for my $fd (@fds) {
+			open($self->{$i++}, '+<&=', $fd) and next;
+			send($sock, "open(+<&=$fd) (FD=$i): $!", MSG_EOR);
+		}
+		return if scalar(@fds) != 4;
 	}
 	$self->{2}->autoflush(1); # keep stdout buffered until x_it|DESTROY
 	# $ENV_STR = join('', map { "\0$_=$ENV{$_}" } keys %ENV);
diff --git a/t/lei-daemon.t b/t/lei-daemon.t
index c30e5ac1..35e059b9 100644
--- a/t/lei-daemon.t
+++ b/t/lei-daemon.t
@@ -2,8 +2,16 @@
 # Copyright (C) 2020-2021 all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 use strict; use v5.10.1; use PublicInbox::TestCommon;
+use Socket qw(AF_UNIX SOCK_SEQPACKET MSG_EOR pack_sockaddr_un);
+use PublicInbox::Spawn qw(which);
 
 test_lei({ daemon_only => 1 }, sub {
+	my $send_cmd = PublicInbox::Spawn->can('send_cmd4') // do {
+		require PublicInbox::CmdIPC4;
+		PublicInbox::CmdIPC4->can('send_cmd4');
+	};
+	$send_cmd or BAIL_OUT 'started testing lei-daemon w/o send_cmd4!';
+
 	my $sock = "$ENV{XDG_RUNTIME_DIR}/lei/5.seq.sock";
 	my $err_log = "$ENV{XDG_RUNTIME_DIR}/lei/errors.log";
 	lei_ok('daemon-pid');
@@ -22,6 +30,27 @@ test_lei({ daemon_only => 1 }, sub {
 	is($pid, $pid_again, 'daemon-pid idempotent');
 	like($lei_err, qr/phail/, 'got mock "phail" error previous run');
 
+	SKIP: {
+		skip 'only testing open files on Linux', 1 if $^O ne 'linux';
+		my $d = "/proc/$pid/fd";
+		skip "no $d on Linux" unless -d $d;
+		my @before = sort(glob("$d/*"));
+		my $addr = pack_sockaddr_un($sock);
+		open my $null, '<', '/dev/null' or BAIL_OUT "/dev/null: $!";
+		my @fds = map { fileno($null) } (0..2);
+		for (0..10) {
+			socket(my $c, AF_UNIX, SOCK_SEQPACKET, 0) or
+							BAIL_OUT "socket: $!";
+			connect($c, $addr) or BAIL_OUT "connect: $!";
+			$send_cmd->($c, \@fds, 'hi',  MSG_EOR);
+		}
+		lei_ok('daemon-pid');
+		chomp($pid = $lei_out);
+		is($pid, $pid_again, 'pid unchanged after failed reqs');
+		my @after = sort(glob("$d/*"));
+		is_deeply(\@before, \@after, 'open files unchanged') or
+			diag explain([\@before, \@after]);;
+	}
 	lei_ok(qw(daemon-kill));
 	is($lei_out, '', 'no output from daemon-kill');
 	is($lei_err, '', 'no error from daemon-kill');

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [SQUASH] [PATCH 1/9] ds: improve DS->Reset fork-safety
  2021-03-24  9:23 ` [PATCH 1/9] ds: improve DS->Reset fork-safety Eric Wong
@ 2021-03-24 23:01   ` Eric Wong
  0 siblings, 0 replies; 11+ messages in thread
From: Eric Wong @ 2021-03-24 23:01 UTC (permalink / raw)
  To: meta

Will squash the following in, no reason to have it accessible
via `our' unless we need to debug something.  And I initally
considered calling it '%state' and stuffing all vars in there,
but decided just on-stack vars would be sufficient.

diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm
index c881c895..3cddfd18 100644
--- a/lib/PublicInbox/DS.pm
+++ b/lib/PublicInbox/DS.pm
@@ -35,7 +35,7 @@ use Errno qw(EAGAIN EINVAL);
 use Carp qw(carp croak);
 our @EXPORT_OK = qw(now msg_more dwaitpid add_timer);
 
-our %Stack;
+my %Stack;
 my $nextq; # queue for next_tick
 my $wait_pids; # list of [ pid, callback, callback_arg ]
 my $later_q; # list of callbacks to run at some later interval
@@ -66,7 +66,6 @@ Reset();
 Reset all state
 
 =cut
-our %state;
 sub Reset {
 	do {
 		$in_loop = undef; # first in case DESTROY callbacks use this

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-03-24 23:01 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-24  9:23 [PATCH 0/9] lei: various corner case leak fixes Eric Wong
2021-03-24  9:23 ` [PATCH 1/9] ds: improve DS->Reset fork-safety Eric Wong
2021-03-24 23:01   ` [SQUASH] " Eric Wong
2021-03-24  9:23 ` [PATCH 2/9] mbox_lock: dotlock: chdir for relative lock paths Eric Wong
2021-03-24  9:23 ` [PATCH 3/9] lei: drop circular reference in lei_store process Eric Wong
2021-03-24  9:23 ` [PATCH 4/9] lei: update {3} after -C chdirs Eric Wong
2021-03-24  9:23 ` [PATCH 5/9] lei: clean up pkt_op consumer on exception, too Eric Wong
2021-03-24  9:23 ` [PATCH 6/9] lei_store: give process a better name Eric Wong
2021-03-24  9:23 ` [PATCH 7/9] v2writable: cleanup SQLite handles on --xapian-only Eric Wong
2021-03-24  9:23 ` [PATCH 8/9] lei_mirror: fix circular reference Eric Wong
2021-03-24  9:23 ` [PATCH 9/9] lei-daemon: do not leak FDs on bogus requests Eric Wong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).