Bug 8030 - spamd SIGHUP restart failure when PERL5LIB is used (t/spamd_hup.t)
Summary: spamd SIGHUP restart failure when PERL5LIB is used (t/spamd_hup.t)
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamc/spamd (show other bugs)
Version: unspecified
Hardware: All All
: P2 normal
Target Milestone: 4.0.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-23 03:31 UTC by Sidney Markowitz
Modified: 2022-08-24 01:58 UTC (History)
4 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Proposed fix for this issue as patch to spamd.raw patch None Sidney Markowitz [HasCLA]
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sidney Markowitz 2022-08-23 03:31:21 UTC 
This is showing up as a test failure in t/spamd_hup.t on a test machine that uses local::lib to install modules for their test environment, which depends on setting the PERL5LIB environment variable to the module install paths. It will break spamd SIGHUP processing on any such machine, not just in tests.

t/SATest.pm contains code to make PERL5LIB work with our tests that run in taint mode (which suppresses passing PERL5LIB to perl) by adding -I options when it uses perl to start up spamd and spamassassin.

This fails in test t/spamd_hup.t when spamd receives a SIGHUP and tries to restart by calling perl to run the spamd script, because it does not pass along the -I options it was started with.

Here is an example report from a CPAN test machine with this problem. I created a custom CPAN module with added debugging output so that this report dumped the spamd STDERR log file when the error occurred. Notice that the spamd debug log has a harmless warning "dbg: geodb: Geo::IP module load failed" that shows @INC when spamd is first running, then later, the fatal error after the restart has "Can't locate NetAddr/IP.pm in @INC" and shows the value of @INC after the restart is missing all the directory paths that were there from PERL5LIB (which is shown in the environment variables near the bottom of the report).

https://www.cpantesters.org/cpan/report/1de2d348-2282-11ed-8f4e-c962458f5e5b

I think the way to fix this is to save @INC into some @ORIGINAL_INC variable immediately at the start of the script, similar to how $ORIGINAL_ARG0 and $ORIGINAL_ARGV are saved, then use that to add -I options to the perl command used at SIGHUP restart. Unfortunately I know of no way to determine the default @INC that is compiled into perl, so those would be redundantly added to @INC. There should be no harm in the restarted process having two copies of those in its @INC. We can remove duplicates in the list of -I options used for the restart call so that subsequent restarts don't keep increasing the number of copies to more than 2.
Comment 1 Sidney Markowitz 2022-08-23 08:06:07 UTC 
Created attachment 5799 [details]
Proposed fix for this issue as patch to spamd.raw

Attached is proposed fix, posted here for an RTC vote.

I reproduced the problem on an Ubuntu VM by doing the following:

I installed CPAN module local::lib and configured it to use ~/perl5 as the local library directory, setting the proper environment variables in ~/.bashrc

I uninstalled NetAddr::IP from my perl site library using cpanm -U NetAddr::IP

Then I installed it with cpanm -l ~/perl5 NetAddr::IP

In svn trunk I ran perl Makefile.PL < /dev/null ; make ; make test TEST_FILES="t/spamd_hup.t" TEST_VERBOSE=1
which reproduced the bug, including the error message in the spamd STDERR log about not being able to find NetAddr::IP in @INC.

After installing this patch, t/spamd_hup.t worked again.

I've uploaded a test build with this patch to my area on CPAN so that the test machines will try it, and should have a useful set of results within 24 hours.

Could committers please review this patch for RTC? It has my +1, need two more.

What it does:

When spamd starts up, in a BEGIN block before anything else has a chance to add anything to @INC, it saves the original contents of @INC into a variable. This will contain whatever paths were added in -I options in the command call to perl, followed by whatever the default @INC of the perl binary is. Unfortunately, I found no way for a running perl to determine what those defaults are other than the pretty nasty step of shelling out a call to perl -V and parsing the output.

Later, after all required modules are loaded, the saved copy of @INC is processed to untaint the paths remove any duplicates and paths that are not absolute or don't exist, and turn it into a series of -I options for the call to restart spamd.

The code that restarts spamd used to read the hashbang line at the top of the spamd script file, and if it matched up with the executable file that perl said it was running would exec the script file directly. That will not work if this issue is a factor, because it will not make use of any -I options that were used when spamd was called. So I removed that code and always invoke spamd on the restart by calling the perl executable, including the -I options.

The restarted spamd will have duplicate paths in @INC for all the paths that are in the perl default. They will be passed in as -I options and prefixed to the default @INC. But perl doesn't do anything bad in tha situation, and the removal of duplicates in the initial code ensures that the number of such duplicates does not increase with each restart.
Comment 2 Giovanni Bechis 2022-08-23 10:33:49 UTC 
+1 for me, regression tests still pass.
Comment 3 Bill Cole 2022-08-23 13:23:09 UTC 
+1
Comment 4 Kevin A. McGrail 2022-08-23 21:38:11 UTC 
Looks good to me. +1 KAM
Comment 5 Sidney Markowitz 2022-08-24 01:58:40 UTC 
trunk % svn ci -m "bug 8030 - Have spamd save incoming @INC to pass as -I options when it does a SIGHUP restart of itself" spamd/spamd.raw 
Sending        spamd/spamd.raw
Transmitting file data .done
Committing transaction...
Committed revision 1903649.