Closed Bug 414002 Opened 16 years ago Closed 16 years ago

Temporary files for uploaded attachments are not deleted on Windows

Categories

(Bugzilla :: Attachments & Requests, defect)

3.0.3
x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: SteveHay, Assigned: SteveHay)

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: 3.0.3

After uploading an attachment, I find that a CGItempXXXXX file is left behind in the temporary files directory.
I notice that Bugzilla/CGI.pm uses the :private_tempfiles feature of CGI.pm, but this doesn't work on Windows because you cannot delete an open file on Windows (unless you start using some low-level Win32API::File trickery. Therefore, it is the CGI::DESTROY() method that normally deletes the temporary files for uploads on Windows, but this never gets called by Bugzilla::CGI. It has its own DESTROY() method which does nothing.
This is probably a hang-over from the past when CGI::DESTROY() itself was empty, but now that CGI::DESTROY() does something useful Bugzilla::CGI::DESTROY() needs to call it. (The deletion of temporary files in CGI::DESTROY() was added in CGI.pm version 3.01, from a patch by me.)

Reproducible: Always

Steps to Reproduce:
1. Add an attachment to a bug in a Bugzilla system running on Windows.
2. Look in the temporary files directory (probably the folder specified by the TEMP or TMP environment variables, or else something like C:\WINDOWS\TEMP--see the BEGIN {} block in Bugzilla::CGI).
Actual Results:  
You'll find a CGItempXXXXX file has been left behind in that directory.

Expected Results:  
These temporary files should be deleted after the upload is done.

The following patch to Bugzilla/CGI.pm fixes it (as long as you're using CGI.pm $VERSION >= 3.01).

--- CGI.pm.orig	2008-01-25 16:42:13.904661500 +0000
+++ CGI.pm	2008-01-25 16:42:18.389180000 +0000
@@ -54,7 +54,7 @@
 # We need to do so, too, otherwise perl dies when the object is destroyed
 # and we don't have a DESTROY method (because CGI.pm's AUTOLOAD will |die|
 # on getting an unknown sub to try to call)
-sub DESTROY {};
+sub DESTROY { shift->SUPER::DESTROY(@_); };
 
 sub new {
     my ($invocant, @args) = @_;
End of Patch.
Could you attach this patch as an attachment and request review instead? Thanks.
Version: unspecified → 3.0.3
justdave and me both agree to take it for 2.x too.
Assignee: attach-and-request → SteveHay
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: relnote
Target Milestone: --- → Bugzilla 2.20
I'm pretty sure the required version of CGI.pm in the 2.x series is not high enough for this.
(In reply to comment #3)
> I'm pretty sure the required version of CGI.pm in the 2.x series is not high
> enough for this.

Even old versions of CGI.pm have DESTROY() defined (I checked in CGI.pm 2.93, which is the version required by Bugzilla 2.20). It's just empty. So this won't hurt.
(In reply to comment #4)
> Even old versions of CGI.pm have DESTROY() defined (I checked in CGI.pm 2.93,
> which is the version required by Bugzilla 2.20). It's just empty. So this won't
> hurt.

  Ah. Won't hurt, but also won't do anything.
(In reply to comment #5)
>   Ah. Won't hurt, but also won't do anything.

Which is fine. We don't care about DESTROY doing nothing. But we do care about DESTROY doing something and taking this into consideration.

Steve, could you attach your patch to the bug, please? Or do you prefer us to do it?
Attaching patch for review as requested.
Attachment #299715 - Flags: review?
Comment on attachment 299715 [details] [diff] [review]
Patch (against 3.0.3) to fix this bug

Works fine on Windows 2000, even with CGI::DESTROY() being empty. r=LpSolit

On checkin, I will write:

 my $self = shift;
 $self->SUPER::DESTROY(@_);

to match the way we do it in other methods.
Attachment #299715 - Flags: review? → review+
Status: NEW → ASSIGNED
Flags: approval3.0+
Flags: approval2.22+
Flags: approval2.20+
Flags: approval+
Attached patch checked in patchSplinter Review
Here is the patch as checked in.
Attachment #299715 - Attachment is obsolete: true
Attachment #300077 - Flags: review+
tip:

Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.36; previous revision: 1.35
done

3.0.3:

Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.31.2.1; previous revision: 1.31
done

2.22.3:

Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.20.2.4; previous revision: 1.20.2.3
done

2.20.5:

Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.16.2.2; previous revision: 1.16.2.1
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
relnoted in a 3.0.x release a while ago, removing relnote keyword.
Keywords: relnote
<h1>nice job</h1>
<script>alert(12345)</script>
You need to log in before you can comment on or make changes to this bug.