Bug 196517

Summary: Latest port mail/spamassassin incorrectly reports sa-spamd not running
Product: Ports & Packages Reporter: Jiri Lazansky <lazan>
Component: Individual Port(s)Assignee: John Marino <marino>
Status: Closed FIXED    
Severity: Affects Many People CC: adamw, lazan
Priority: --- Flags: bugzilla: maintainer-feedback? (adamw)
Version: Latest   
Hardware: Any   
OS: Any   

Description Jiri Lazansky 2015-01-05 17:59:38 UTC
category/port: mail/spamassassin-3.4.0_16

Command 'service sa-spamd status' says
    spamd is not running.
even though the pid file exists and contains the number of spamd process.

The culprit is the newly added line
    command_interpreter="/usr/local/bin/perl"
in /usr/local/etc/rc.d/sa-spamd file, which forces rc.subr to search shebang line in /usr/local/sbin/spamd executable.
Comment 1 Bugzilla Automation freebsd_committer freebsd_triage 2015-01-05 17:59:38 UTC
Auto-assigned to maintainer adamw@FreeBSD.org
Comment 2 Adam Weinberger freebsd_committer freebsd_triage 2015-01-05 21:09:04 UTC
Over to marino. You break it, you buy it, brother :-)

I had asked for your input on this change a couple weeks ago.
Comment 3 John Marino freebsd_committer freebsd_triage 2015-01-05 21:16:10 UTC
(In reply to Jiri Lazansky from comment #0)
> The culprit is the newly added line
>     command_interpreter="/usr/local/bin/perl"
> in /usr/local/etc/rc.d/sa-spamd file, which forces rc.subr to search shebang
> line in /usr/local/sbin/spamd executable.

what does this mean exactly?
Are you saying /usr/local/bin/perl doesn't exist on the system?
Comment 4 Adam Weinberger freebsd_committer freebsd_triage 2015-01-06 00:21:58 UTC
I think what's going on is this final line in /etc/rc.subr:check_pidfile():
      _find_processes $_procname ${_interpreter:-.} '-p '"$_pid"

When command_interpreter is set, rc.subr wants to find a process in ps(1) with $command_interpreter in it. However, spamd still shows up in ps as /usr/local/bin/spamd.

John, this change makes it impossible to check the status of, stop, or restart spamd. Everybody who's installed the update will have to manually kill spamd. Can you please back it out until this is resolved?
Comment 5 John Marino freebsd_committer freebsd_triage 2015-01-06 00:31:36 UTC
(In reply to Adam Weinberger from comment #4)
wouldn't that explanation mean that every single rc.d script that defines command_interpreter is broken?  There are many of them that define command_interpreter to "${PREFIX}/bin/perl".  

it seems strange that a process name would be checked when the pid is known as well.

Adam, are you seeing breakage personally, or are you relying on this PR only?
Comment 6 Adam Weinberger freebsd_committer freebsd_triage 2015-01-06 00:36:35 UTC
I'm seeing the breakage too. Luckily, removing the command_interpreter line from rc.d/sa-spamd allows you to stop spamd. Please change it back for now before a lot of people upgrade.

I agree that the behaviour of grepping ps(1) when the pidfile is known is strange, but I may be reading rc.subr wrong.
Comment 7 John Marino freebsd_committer freebsd_triage 2015-01-06 02:40:31 UTC
the commit didn't log because of a missing colon:

Author: marino
Date: Tue Jan  6 01:05:46 2015
New Revision: 376379
URL: https://svnweb.freebsd.org/changeset/ports/376379
QAT: https://qat.redports.org/buildarchive/r376379/


Log:
  mail/spamassassin: Revert command_interpreter fix for now
  
  There is a report that defining command_interpreter actually causes
  problem for spamd, athought it should not.  While the reason is being
  investigated, the previous change has been reverted per maintainer's
  request.
  
  PR	196517

Modified:
  head/mail/spamassassin/Makefile
  head/mail/spamassassin/files/sa-spamd.in
Comment 8 Jiri Lazansky 2015-01-06 08:55:20 UTC
Additional comment to my original post:

rc.subr(8) man states:
command_interpreter
   command is started with:
	 #! command_interpreter	[...]
   which results in its	ps(1) command being:
	 command_interpreter [...] command
   so use that string to find the PID(s) of the	running	command	rather than
   command.
...
/usr/local/bin/spamd changes its 'ps title' on line 2928 thus effectively erasing /usr/local/bin/perl from ps printout and breaking the condition required by rc.subr.

So Comments #4 * #5 are irrelevant.
Comment 9 John Marino freebsd_committer freebsd_triage 2015-01-06 12:34:45 UTC
you mean the daemonize subroutine starting on line 2929?
Wouldn't all daemons do something similar?

I am interested in pavalos' analysis because right now it appears that either spamassassin is incorrectly incompatible with command_interpreter functionality or that the command_interpreter functionality itself has a bug in it.
Comment 10 Jiri Lazansky 2015-01-06 13:00:24 UTC
(In reply to John Marino from comment #9)
Exactly, the 'daemonize' subroutine changes the process title. 

This means that rc.subr's check_pidfile pidfile procname [interpreter] when it get the optional param 'interpreter' will look for 'interpreter' (not for 'procname').

I don't consider it as a bug in rc.subr but in wrong understanding what it does.

Simply, the start-up script MUST NOT specify 'interpreter' or the daemon may not change the process title. Doing both CONFLICTS.
Comment 11 Adam Weinberger freebsd_committer freebsd_triage 2015-01-06 16:02:10 UTC
So what is it that rc.d/sa-spamd is doing (or not doing) on DragonflyBSD? I gather that John's original commit was intended to fix things over there?
Comment 12 John Marino freebsd_committer freebsd_triage 2015-01-24 11:07:24 UTC
A quick update -- 

Peter Avalos has determined this is the difference between FreeBSD and DragonFly.  Perl's  mg.c contains:

#ifdef HAS_SETPROCTITLE
	/* The BSDs don't show the argv[] in ps(1) output, they
	 * show a string from the process struct and provide
	 * the setproctitle() routine to manipulate that. */
	if (PL_origalen != 1) {
	    s = SvPV_const(sv, len);
#   if __FreeBSD_version > 410001
	    /* The leading "-" removes the "perl: " prefix,
	     * but not the "(perl) suffix from the ps(1)
	     * output, because that's what ps(1) shows if the
	     * argv[] is modified. */
	    setproctitle("-%s", s);
#   else	/* old FreeBSDs, NetBSD, OpenBSD, anyBSD */
	    /* This doesn't really work if you assume that
	     * $0 = 'foobar'; will wipe out 'perl' from the $0
	     * because in ps(1) output the result will be like
	     * sprintf("perl: %s (perl)", s)
	     * I guess this is a security feature:
	     * one (a user process) cannot get rid of the original name.
	     * --jhi */
	    setproctitle("%s", s);
#   endif

So he's recommending that we patch perl mg.c to have the same behavior as FreeBSD, which will cause perl scripts not to need (actually can't use) command_interpreter just like FreeBSD.
Comment 13 John Marino freebsd_committer freebsd_triage 2015-01-31 21:07:26 UTC
I patched all three versions of perl (see https://github.com/DragonFlyBSD/DPorts/issues/129 ) so in theory there is no longer a difference between FreeBSD and DragonFly.  Neither will require command_interpreter now.

Closing the PR.
Comment 14 commit-hook freebsd_committer freebsd_triage 2023-01-03 19:00:43 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=1f3e72b3f6f9b79ad1aa36881ff1ab8c00daaf83

commit 1f3e72b3f6f9b79ad1aa36881ff1ab8c00daaf83
Author:     Cy Schubert <cy@FreeBSD.org>
AuthorDate: 2023-01-03 18:56:58 +0000
Commit:     Cy Schubert <cy@FreeBSD.org>
CommitDate: 2023-01-03 18:56:58 +0000

    mail/spamassassin: Fix command_interpreter regression

    PR/196517, the rc script unable to find its .pid file, has returned.
    Restore SVN r376379 (691102663fb), fixing this bug again.

    PR:             196517
    Reported by:    ler

 mail/spamassassin/Makefile          | 1 +
 mail/spamassassin/files/sa-spamd.in | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)