SA Bugzilla – Bug 4017
tmpfile creation has infinite loop bug if it runs into fd ulimit or disk quota limit
Last modified: 2005-04-29 07:58:38 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); }
Created attachment 2546 [details] here's the patch
Please use the unified format (diff -u) to create the patch. Thanks.
*** Bug 4016 has been marked as a duplicate of this bug. ***
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.
(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
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...
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.