Created attachment 4957 [details]
patch doublechecks dir existance and write perms.

without patch, it does, well, nothing good.

add patch:
first, tested on nonexistant dir:

set TMPDIR=/TMPDIR
sa-update -D

ug 26 17:06:12.617 [87393] dbg: gpg: found /usr/local/bin/gpg
Aug 26 17:06:12.617 [87393] dbg: gpg: release trusted key id list: 5E541DC959CB8BAC7C78DFDC4056A61A5244EC45 26C900A46DD40CD5AD24F6D7DEE01987265FA05B 0C2B1D7175B852C64B3CDC716C55397824F434CE
Could not open /TMPDIR: No such file or directory at /usr/local/lib/perl5/site_perl/5.10.1/Mail/SpamAssassin/Util.pm line 1027.


then, set path to readonly dir:

cd /usr/bin
# touch t.t
touch: t.t: Read-only file system
# TMPDIR=/usr/bin
# export TMPDIR

til: secure_tmpfile failed to create file '/usr/bin/.spamassassin89328CaYVrOtmp': Read-only file system
Aug 26 17:15:16.676 [89328] info: error closing /usr/bin/.spamassassin89328CaYVrOtmp: Bad file descriptor
util: secure_tmpfile failed to create file, giving up at /usr/local/lib/perl5/site_perl/5.10.1/Mail/SpamAssassin/Util.pm line 1066.
fatal: could not create temporary channel content file:

Created attachment 4958 [details]
replacement patch.

patches ../lib/Mail/SpamAssassin/Util.pm to check for existance of, and read perms on TMPDIR, dies if not.

patches ../sa-update.raw. TRY to catch errors on fsys full on TMPDIR.

(are tracking down why several sites running sa-update on 3.3.1 are pulling sa-updates 9000 per day.  3000 per day per mirror.)

(In reply to comment #2)
> Created attachment 4958 [details]
> replacement patch.
> 
> patches ../lib/Mail/SpamAssassin/Util.pm to check for existance of, and read
> perms on TMPDIR, dies if not.
> 
> patches ../sa-update.raw. TRY to catch errors on fsys full on TMPDIR.
> 
> (are tracking down why several sites running sa-update on 3.3.1 are pulling
> sa-updates 9000 per day.  3000 per day per mirror.)

KAM: Tested and logic seems good +1 for me for 3.3. 

Committing to trunk:

svn commit -m 'Michaels patch for more error trapping on sa-update.  See bug 6655'
Sending        lib/Mail/SpamAssassin/Util.pm
Sending        sa-update.raw
Transmitting file data ..
Committed revision 1163848.

Comment #2:  "...[T]o check for existance of, and read perms on TMPDIR...."

I believe the patch is correctly looking for WRITE permission too....

Intended as a quick test of the supplied patch and to explore some other
failure modes, and realizing that plenty of system calls in sa-update
were still ignoring a status return, I ended up with a biggish update
to sa-update and some of the related utility code. I believe it is
an improvement, but further improvement would still be desirable.

Here are the changes:
- sa-update can now take multiple -v or --verbose options to increase
  verbosity; currently one or two levels are in use: two levels
  add reporting on a DNS query and HTTP GET requests;
- allow Mail::SpamAssassin::Util::secure_tmpfile to signal a
  failure, adjust callers for this;
- Message.pm: test $msg->{'raw'} for defined(), not for exists();
  do not store a file handle there if it is not defined;
- sub secure_tmpfile: only retry on failures which are expected
  not to be permanent (e.g. makes no point in trying 20 times
  to create a file when a status is always a "Permission denied";
- sub secure_tmpfile: do not bother with umask, sysopen specifies
  the mode explicitly;
- sa-update: add several missing status tests to system calls,
  improve debug and verbose logging;
- sa-update: use eval{} in the 'try/rollback' idiom to simplify
  diagnostics;
- sa-update: ignore non-'TXT' and empty fields in a DNS reply;
- sa-update: better report gpg process crashes and failure exit status;
- sa-update: some style and terminology changes to go along the
  rest of the modules (like: || warn -> or warn; can't -> cannot);

trunk:
  Sending lib/Mail/SpamAssassin/Message.pm
  Sending lib/Mail/SpamAssassin/PerMsgStatus.pm
  Sending lib/Mail/SpamAssassin/Plugin/BodyRuleBaseExtractor.pm
  Sending lib/Mail/SpamAssassin/Util.pm
  Sending sa-update.raw
Committed revision 1165372.



Here is an example of a failure mode that would still cause
repeated cron updates with rules tar files being repeatedly
downloaded despite a previous success:

DNS TXT query: 0.4.3.updates.spamassassin.org -> 1165208
  Update available for channel updates.spamassassin.org
DNS TXT query: mirrors.updates.spamassassin.org ->
  http://spamassassin.apache.org/updates/MIRRORED.BY
http: GET http://spamassassin.apache.org/updates/MIRRORED.BY, 200 OK
http: GET http://sa-update.secnap.net/1165208.tar.gz, 200 OK
http: GET http://sa-update.secnap.net/1165208.tar.gz.sha1, 200 OK
error: failed to create /var/lib/spamassassin/3.004000/
  updates_spamassassin_org/10_default_prefs.cf:
  Permission denied at /usr/local/bin/sa-update line 1132.
channel: archive extraction failed, channel failed
Update failed, exiting with code 4

> Here is an example of a failure mode that would still cause
> repeated cron updates with rules tar files being repeatedly
> downloaded despite a previous success:
> 
> DNS TXT query: 0.4.3.updates.spamassassin.org -> 1165208
>   Update available for channel updates.spamassassin.org
> DNS TXT query: mirrors.updates.spamassassin.org ->
>   http://spamassassin.apache.org/updates/MIRRORED.BY
> http: GET http://spamassassin.apache.org/updates/MIRRORED.BY, 200 OK
> http: GET http://sa-update.secnap.net/1165208.tar.gz, 200 OK
> http: GET http://sa-update.secnap.net/1165208.tar.gz.sha1, 200 OK
> error: failed to create /var/lib/spamassassin/3.004000/
>   updates_spamassassin_org/10_default_prefs.cf:
>   Permission denied at /usr/local/bin/sa-update line 1132.
> channel: archive extraction failed, channel failed
> Update failed, exiting with code 4

We were discussing this scenario a few days ago and my thoughts were, what about not deleting the sa-update files and using a standard directory for the files so they weren't continually downloaded over and over?

> We were discussing this scenario a few days ago and my thoughts were, what
> about not deleting the sa-update files and using a standard directory for the
> files so they weren't continually downloaded over and over?

Yes, that would make sense - it is a straightforward way to represent
a simple state machine.  Sorry for not following closely.

(In reply to comment #7)
> > We were discussing this scenario a few days ago and my thoughts were, what
> > about not deleting the sa-update files and using a standard directory for the
> > files so they weren't continually downloaded over and over?
> 
> Yes, that would make sense - it is a straightforward way to represent
> a simple state machine.  Sorry for not following closely.

Might have been an out of band discussion so no apology necessary.

As a reminder: decoupling download from further unpacking and installation
of rules still needs to be done (so that a download is not repeated if
the installation of rules fails).

Created attachment 4963 [details]
Patch from Mark for Review

> Created attachment 4963 [details]
> Patch from Mark for Review

Note that this patch just improves error detection and improves
some minor points. The core of the problem is still unhandled.

Copying the comment from the related Bug 6654:


I'm committing my current state of sa-update for review/comments/testing.
It turned out to be a much more work than anticipated. It may still need
one or two details to be ironed out, but should already be usable and
should give an idea of what the change is all about.

From the mailing list: 2011-11-05 Mark wrote:
> I have the first part of the sa-update change mostly ready, will 
> commit for review possibly tonight or tomorrow. Giving up on the LWP 
> module (but keeping it as a fallback), but using external programs 
> curl or wget of fetch preferentially, as they all know how to deal 
> with IPv4/IPv6 and A/AAAA/CNAME properly, unlike the libwww-perl which 
> seems to be ignoring IPv6 support requests persistently.

Kevin wrote:
> Sounds like a sane solution.

Mark wrote:
> What remains is to deal with avoiding repeated downloads if
> later stages of rules installation fails. Not sure how to go
> about it (and where to keep downloaded files).


As a solution to both Bug 6655 and Bug 6654 is intertwined, I did not
try to keep the changes apart, so here is a single commit:

trunk:

  Bug 6655: sa-update might DOS mirrors if TMPDIR unwritable
  Bug 6654: Make sa-update and its infrastructure usable over IPv6

- the LWP perl module does not support IPv6 and is hard to trick properly
  to use IO::Socket::INET6 in place of IO::Socket::INET (e.g., doesn't
  handle multihomed (like dual-stack) hosts).  Instead, add code to use
  one of the external programs (curl, wget, fetch) to download rules files
  and mirrors file, and make LWP module optional. If an external program
  is not available, fall back to LWP;

- preserve .tar.gz, .sha1 and .asc files if rules installation fails,
  so that a subsequent attempt does not download these files again,
  as long as their size and creation time match the server
  (this task is handled by curl, wget, or fetch);

- improve debug logging;

Sending sa-update.raw
Committed revision 1199081.

This is committed and considered resolved.

Are there any notes in the INSTALL or some such to let people know that having
one of curl, wget or fetch installed is a good idea? Solaris 10 does have wget
in /usr/sfw/bin, but that is not in the path by default.

(In reply to comment #14)
> Are there any notes in the INSTALL or some such to let people know that
> having
> one of curl, wget or fetch installed is a good idea? Solaris 10 does have
> wget
> in /usr/sfw/bin, but that is not in the path by default.

Good point!

I've added code to the INSTALL and to DependencyInfo.pm to check for the binaries.

IMPORTANT: Anyone have fetch or can point me to a link to donwnload so I can identify a good recommend version and the regex check for the version info?  

Index: INSTALL
===================================================================
--- INSTALL     (revision 1428441)
+++ INSTALL     (working copy)
@@ -89,6 +89,10 @@
    See the "Installing Rules" section below if you do not wish to download
    the rules directly from the internet.
 
+   NOTE: Because LWP does not support IPv6, sa-update as of 3.4.0 will use
+   the binaries curl, wget or fetch to download rule updates with LWP used 
+   as a fallback if none of the binaries exist.
+
 5. If you already use procmail, skip to step 7.  If not, ensure procmail
    is installed using "which procmail" or install it from www.procmail.org.
 
Index: lib/Mail/SpamAssassin/Util/DependencyInfo.pm
===================================================================
--- lib/Mail/SpamAssassin/Util/DependencyInfo.pm        (revision 1428441)
+++ lib/Mail/SpamAssassin/Util/DependencyInfo.pm        (working copy)
@@ -258,6 +258,10 @@
 
 my @BINARIES = ();
 
+my $lwp_note = "   Because LWP does not support IPv6, sa-update as of 3.4.0 will use
+   the binaries curl, wget or fetch to download rule updates with LWP used 
+   as a fallback if none of the binaries exist.";
+
 my @OPTIONAL_BINARIES = (
 {
   binary => 'gpg',
@@ -268,6 +272,23 @@
   desc => 'The "sa-update" program requires this executable to verify  
   encryption signatures.  It is not recommended, but you can use 
   "sa-update" with the --no-gpg to skip the verification. ',
+},
+{
+  binary => 'wget',
+  version => '0',
+  recommended_min_version => '1.8.2',
+  version_check_params => '--version',
+  version_check_regex => 'Gnu Wget ([\d\.]*)',
+  desc => $lwp_note,
+},
+
+# I DO NOT HAVE FETCH TO CHECK.
+{
+  binary => 'fetch',
+  version => '0',
+  version_check_params => '--version',
+  version_check_regex => 'fetch ([\d\.]*)',
+  desc => $lwp_note,
 }
 );

svn commit -m 'Documentation in INSTALL and DependencyInfo.pm because sa-update now uses binaries before LWP for better ipv6 support - bug 6655'
Sending        INSTALL
Sending        lib/Mail/SpamAssassin/Util/DependencyInfo.pm
Transmitting file data ..
Committed revision 1428581.


Leaving open to identify the information for fetch.  Only fetch program I know is a commercial proggy for macs.  Is that the one we are talking about?

> IMPORTANT: Anyone have fetch or can point me to a link to donwnload so I can
> identify a good recommend version and the regex check for the version info?  

> Leaving open to identify the information for fetch.  Only fetch program I
> know is a commercial proggy for macs.  Is that the one we are talking about?

The fetch command comes with a base FreeBSD system and is a default for
all system updates (ports updates mainly).

Its man page says:

HISTORY
  The fetch command appeared in FreeBSD 2.1.5.  This implementation first
  appeared in FreeBSD 4.1.

AUTHORS
  The original implementation of fetch was done by Jean-Marc Zucconi
  jmz@FreeBSD.org>.  It was extensively re-worked for FreeBSD 2.2 by
  Garrett Wollman <wollman@FreeBSD.org>, and later completely rewritten to
  use the fetch(3) library by Dag-Erling Smorgrav <des@FreeBSD.org>.

I cleaned up the DependencyInfo for Curl and Fetch.  Since Fetch has no version command line parameter, I also added code to handle this scenario.

svn commit -m 'Cleaned up DependencyInfo for Curl and Fetch.'   
Sending        lib/Mail/SpamAssassin/Util/DependencyInfo.pm
Transmitting file data .
Committed revision 1428720.

Regards,
KAM