Bug 4017 - tmpfile creation has infinite loop bug if it runs into fd ulimit or disk quota limit
Summary: tmpfile creation has infinite loop bug if it runs into fd ulimit or disk quot...
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamassassin (show other bugs)
Version: 3.0.0
Hardware: Other other
: P1 major
Target Milestone: 3.1.0
Assignee: Justin Mason
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-12-03 15:06 UTC by Jay Rogers
Modified: 2005-04-29 07:58 UTC (History)
1 user (show)



Attachment Type Modified Status Actions Submitter/CLA Status
here's the patch patch None Jay Rogers [NoCLA]

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Rogers 2004-12-03 15:06:40 UTC
Spamassassin v3.0.0 appears to be using an excessive amount
of open file descriptors for dns use.  This is likely a bug.
The exact amount depends on the message being processed.
For example, the particular spam I've been testing with
below needs at least 97 open fds to successfully complete.
The process limit is 64.

Once you hit the fd limit, there's the potential of getting
stuck in an infinite loop (i.e. some code continuely retries
opening a file on errno "too many open files").

You can tell if you've hit this bug by the following error
in your procmail log:

    procmail: Rescue of unfiltered data succeeded
    procmail: Timeout, terminating "/usr/local/bin/spamassassin.new"
    procmail: Kernel-lock failed
    procmail: Kernel-unlock failed

Here's a patch:

*** /usr/local/test-sa/lib/site_perl/5.8.0/Mail/SpamAssassin/Util.pm	Mon Sep 13
19:34:05 2004
--- Mail/SpamAssassin/Util.pm	Fri Dec  3 15:31:59 2004
***************
*** 724,766 ****
  
  ###########################################################################
  
- # thanks to http://www2.picante.com:81/~gtaylor/autobuse/ for this
- # code.
  sub secure_tmpfile {
!   my $tmpdir = Mail::SpamAssassin::Util::untaint_file_path(
!                  File::Spec->tmpdir()
!                );
!   if (!$tmpdir) {
!     die "Cannot find a temporary directory! set TMP or TMPDIR in env";
!   }
  
!   my ($reportfile,$tmpfile);
!   my $umask = umask 077;
!   do {
      # we do not rely on the obscurity of this name for security...
      # we use a average-quality PRG since this is all we need
!     my $suffix = join ('',
!                        (0..9, 'A'..'Z','a'..'z')[rand 62,
!                                                  rand 62,
!                                                  rand 62,
!                                                  rand 62,
!                                                  rand 62,
!                                                  rand 62]);
!     $reportfile = File::Spec->catfile(
!                     $tmpdir,
!                     join ('.',
!                       "spamassassin",
!                       $$,
!                       $suffix,
!                       "tmp",
!                     )
!                   );
      # ...rather, we require O_EXCL|O_CREAT to guarantee us proper
      # ownership of our file; read the open(2) man page.
!   } while (! sysopen ($tmpfile, $reportfile, O_RDWR|O_CREAT|O_EXCL, 0600));
!   umask $umask;
  
!   return ($reportfile, $tmpfile);
  }
  
  ###########################################################################
--- 724,766 ----
  
  ###########################################################################
  
  sub secure_tmpfile {
!   my ($reportfile, $suffix, $tmpdir, $tmpfile, $umask);
!   my $count = 0;
! 
!   $tmpdir = Mail::SpamAssassin::Util::untaint_file_path(File::Spec->tmpdir)
!       or die "Cannot find a temporary directory! set TMP or TMPDIR in env";
! 
!   $umask = umask 077;
  
!   while (1) {
      # we do not rely on the obscurity of this name for security...
      # we use a average-quality PRG since this is all we need
!     $suffix = join ('', (0..9,'A'..'Z','a'..'z')[rand 62,
! 						 rand 62,
! 						 rand 62,
! 						 rand 62,
! 						 rand 62,
! 						 rand 62]);
!     $reportfile = File::Spec->catfile($tmpdir, join('.',
! 						    "spamassassin",
! 						    $$,
! 						    $suffix,
! 						    "tmp"));
!     die "Problem creating temporary file" if ++$count > 100_000;
! 
!     next if -e $reportfile;
! 
      # ...rather, we require O_EXCL|O_CREAT to guarantee us proper
      # ownership of our file; read the open(2) man page.
!     sysopen $tmpfile, $reportfile, O_RDWR|O_CREAT|O_EXCL, 0600
! 	or die "Cannot create temporary file \"$reportfile\": $!";
! 
!     last;
!   }
  
!   umask $umask;
!   return($reportfile, $tmpfile);
  }
Comment 1 Jay Rogers 2004-12-03 15:15:39 UTC
Created attachment 2546 [details]
here's the patch
Comment 2 Malte S. Stretz 2004-12-03 15:47:18 UTC
Please use the unified format (diff -u) to create the patch.  Thanks. 
Comment 3 Malte S. Stretz 2004-12-03 15:48:29 UTC
*** Bug 4016 has been marked as a duplicate of this bug. ***
Comment 4 Justin Mason 2005-04-13 10:15:28 UTC
actually this is a major bug and needs to be fixed, alright.  however the patch
is incorrect; EEXIST is expected occasionally, and that's what that code is
doing; trying to retry if it gets an EEXIST. so a different fix will be required.
Comment 5 Daniel Quinlan 2005-04-29 01:30:31 UTC
(Justin) tag, you're it

Jay, feel free to try again taking into account Justin's comments.  ;-)
If we do take your patch, we should probably get an Apache CLA for
you, though.  Could you sign and fax one if you do attach a new atch?

http://www.apache.org/licenses/#clas
Comment 6 Justin Mason 2005-04-29 11:32:07 UTC
nah, it's OK, I can fix that here using that patch as a baseline for ideas, I
don't need a new patch.  taking bug...
Comment 7 Justin Mason 2005-04-29 15:58:38 UTC
ok, fixed in 3.1.0; better (non-infinite) retry code in this fn, explicit test
for EEXIST, and less fds used in general.

r165354.