Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Temporary files for uploaded attachments are not deleted on Windows

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Attachments & Requests
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Steve Hay, Assigned: Steve Hay)

Tracking

3.0.3
Bugzilla 2.20
x86
Windows XP
Bug Flags:
approval +
approval3.0 +
approval2.22 +
approval2.20 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

682 bytes, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.

Comment 1

10 years ago
Could you attach this patch as an attachment and request review instead? Thanks.
Version: unspecified → 3.0.3

Comment 2

10 years ago
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

Comment 3

10 years ago
I'm pretty sure the required version of CGI.pm in the 2.x series is not high enough for this.

Comment 4

10 years ago
(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.

Comment 5

10 years ago
(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.

Comment 6

10 years ago
(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?
(Assignee)

Comment 7

10 years ago
Created attachment 299715 [details] [diff] [review]
Patch (against 3.0.3) to fix this bug

Attaching patch for review as requested.
Attachment #299715 - Flags: review?

Comment 8

10 years ago
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+

Updated

10 years ago
Status: NEW → ASSIGNED
Flags: approval3.0+
Flags: approval2.22+
Flags: approval2.20+
Flags: approval+

Comment 9

10 years ago
Created attachment 300077 [details] [diff] [review]
checked in patch

Here is the patch as checked in.
Attachment #299715 - Attachment is obsolete: true
Attachment #300077 - Flags: review+

Comment 10

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 11

9 years ago
relnoted in a 3.0.x release a while ago, removing relnote keyword.
Keywords: relnote

Comment 12

5 years ago
<h1>nice job</h1>

Comment 13

5 years ago
<script>alert(12345)</script>
You need to log in before you can comment on or make changes to this bug.