Last Comment Bug 414002 - Temporary files for uploaded attachments are not deleted on Windows
: Temporary files for uploaded attachments are not deleted on Windows
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: 3.0.3
: x86 Windows XP
: -- normal (vote)
: Bugzilla 2.20
Assigned To: Steve Hay
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-25 09:04 PST by Steve Hay
Modified: 2012-03-25 18:59 PDT (History)
4 users (show)
LpSolit: approval+
LpSolit: approval3.0+
LpSolit: approval2.22+
LpSolit: approval2.20+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (against 3.0.3) to fix this bug (546 bytes, patch)
2008-01-28 00:34 PST, Steve Hay
LpSolit: review+
Details | Diff | Splinter Review
checked in patch (682 bytes, patch)
2008-01-29 10:57 PST, Frédéric Buclin
LpSolit: review+
Details | Diff | Splinter Review

Description Steve Hay 2008-01-25 09:04:56 PST
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 Frédéric Buclin 2008-01-25 10:09:33 PST
Could you attach this patch as an attachment and request review instead? Thanks.
Comment 2 Frédéric Buclin 2008-01-25 11:52:11 PST
justdave and me both agree to take it for 2.x too.
Comment 3 Max Kanat-Alexander 2008-01-26 00:10:25 PST
I'm pretty sure the required version of CGI.pm in the 2.x series is not high enough for this.
Comment 4 Frédéric Buclin 2008-01-26 02:30:56 PST
(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 Max Kanat-Alexander 2008-01-26 15:53:07 PST
(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 Frédéric Buclin 2008-01-27 09:36:37 PST
(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?
Comment 7 Steve Hay 2008-01-28 00:34:17 PST
Created attachment 299715 [details] [diff] [review]
Patch (against 3.0.3) to fix this bug

Attaching patch for review as requested.
Comment 8 Frédéric Buclin 2008-01-28 11:19:17 PST
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.
Comment 9 Frédéric Buclin 2008-01-29 10:57:43 PST
Created attachment 300077 [details] [diff] [review]
checked in patch

Here is the patch as checked in.
Comment 10 Frédéric Buclin 2008-01-29 11:24:10 PST
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
Comment 11 Max Kanat-Alexander 2008-06-30 23:58:17 PDT
relnoted in a 3.0.x release a while ago, removing relnote keyword.
Comment 12 sabb 2012-03-25 18:58:48 PDT
<h1>nice job</h1>
Comment 13 sabb 2012-03-25 18:59:26 PDT
<script>alert(12345)</script>

Note You need to log in before you can comment on or make changes to this bug.