• Do not register here on develop.twiki.org, login with your twiki.org account.
• Use View topic Item7848 for generic doc work for TWiki-6.1.1. Use View topic Item7851 for doc work on extensions that are not part of a release. More... Close
• Anything you create or change in standard webs (Main, TWiki, Sandbox etc) will be automatically reverted on every SVN update.
Does this site look broken?. Use the LitterTray web for test cases.

Item5307: Plugins with beforeAttachmentSaveHandler break file attachments with TWiki 4.2.0

Item Form Data

AppliesTo: Component: Priority: CurrentState: WaitingFor: TargetRelease ReleasedIn
Engine   Urgent Closed   patch 4.2.1, 5.0.0

Edit Form Data

Summary:
Reported By:
Codebase:
Applies To:
Component:
Priority:
Current State:
Waiting For:
Target Release:
Released In:
 

Detail

I'm running TWiki 4.2.0 (Apache 2.2 - Debian Etch) and BlackListPlugin r13238. I can no longer attach files to TWiki topics. The file is uploaded, but it's zero bytes in size on the server. Apache's error log shows these errors
binmode() on closed filehandle fh00001 ... at lib/TWiki/Store.pm line 976... 
read() on closed filehandle ... at lib/TWiki/Store/RcsFile.pm line 730

Disabling BlackListPlugin in configure gets the upload working again.

-- TWiki:Main/MartinRowe - 31 Jan 2008

Item5265 has RevCommentPlugin showing the same behaviour.

-- TWiki:Main.SteffenPoulsen - 31 Jan 2008

After having looked at the code in he core, as well as the two plugin I am convinced the problem is in the core.

Looking at the svn blame lib/TWiki/Store.pm we can see something was changed with the 14070 checkin that could be important.

  6065 CrawfordCurrie             if( $plugins->haveHandlerFor( 'beforeAttachmentSaveHandler' )) {
 14070 CrawfordCurrie                 # Because of the way CGI works, the stream is actually attached
 14070 CrawfordCurrie                 # to a file that is already on disc. So all we need to do
 14070 CrawfordCurrie                 # is determine that filename, close the stream, process the
 14070 CrawfordCurrie                 # upload and then reopen the stream on the resultant file.
 14070 CrawfordCurrie                 close( $opts->{stream} );
 14070 CrawfordCurrie                 if (!defined($attrs->{tmpFilename})) {
 14070 CrawfordCurrie                     throw Error::Simple(
 14070 CrawfordCurrie                         "Cannot call beforeAttachmentSaveHandler; CGI did not provide a temporary file name");
  6065 CrawfordCurrie                 }
  6065 CrawfordCurrie                 $plugins->beforeAttachmentSaveHandler( $attrs, $topic, $web );
 14070 CrawfordCurrie                 open( $opts->{stream}, "<$attrs->{tmpFilename}" );
  6065 CrawfordCurrie                 binmode( $opts->{stream} );
  6065 CrawfordCurrie             }

Looking at the 14070 checkin it was a bundle of quite many changes so tracking is a bit of a challenge.

The relevant changes to this are in Store.pm

-            my $tmpFile;
-
             if( $plugins->haveHandlerFor( 'beforeAttachmentSaveHandler' )) {
-                # SMELL: legacy spec of beforeAttachmentSaveHandler requires
-                # a local copy of the stream. This could be a problem for
-                # very big data files.
-                use File::Temp;
-                my $fh;
-                # Note: do *not* rely on UNLINK => 1, because in a mod_perl
-                # context the destructor may not be called for a *long* time.
-                # Call tempfile in a list context so that file does not get
-                # deleted when closed.
-                ( $fh, $tmpFile ) = File::Temp::tempfile();
-                binmode( $fh );
-                # transfer 512KB blocks
-                my $transfer;
-                my $r;
-                while( $r = sysread( $opts->{stream}, $transfer, 0x80000 )) {
-                    syswrite( $fh, $transfer, $r );
+                # Because of the way CGI works, the stream is actually attached
+                # to a file that is already on disc. So all we need to do
+                # is determine that filename, close the stream, process the
+                # upload and then reopen the stream on the resultant file.
+                close( $opts->{stream} );
+                if (!defined($attrs->{tmpFilename})) {
+                    throw Error::Simple(
+                        "Cannot call beforeAttachmentSaveHandler; CGI did not provide a temporary file name");
                 }
-                close( $fh );
-                $attrs->{tmpFilename} = $tmpFile;
                 $plugins->beforeAttachmentSaveHandler( $attrs, $topic, $web );
-                open( $opts->{stream}, "<$tmpFile" );
+                open( $opts->{stream}, "<$attrs->{tmpFilename}" );
                 binmode( $opts->{stream} );
             }

The thing BlackListPlugin and RevCommentPlugin has in common is that they have a beforeAttachmentSaveHandler. And I believe that any plugin just having this handler - even if it does nothing triggers the problem.

I run a TWiki with both RevCommentPlugin and BlackListPlugin active and I can still attach files.

The problem could be related to the CGI version. It could also be related to how the file system behind behaves. Maybe even the type of file system. Is the assumption "Because of the way CGI works, the stream is actually attached to a file that is already on disc" always true?

Martin can you give some details on your environment. You are on debian etch. Which file system? And what is the version of CGI you have installed?

perl -e 'use CGI; print "$CGI::VERSION\n";' will tell you the version of CGI

Remember to put back report in "New" when you have answered.

-- TWiki:Main.KennethLavrsen - 31 Jan 2008

CGI is 3.15. File system is ext3. You can see the plugin details for the site at http://www.dbg400.net/cgi-bin/twiki/view/TWiki/InstalledPlugins

-- TWiki:Main.MartinRowe - 31 Jan 2008

Earlier (and not guaranteed relevant) items on similar subject: Item2097, Item2371 and Item2390.

-- TWiki:Main.SteffenPoulsen - 31 Jan 2008

I also use CGI 3.15, and ext3 filesystem. So it is something else.

I also found Item2390 which is EXACTLY the same error symptom.

The fix from then is what was changed with 14070.

-- TWiki:Main.KennethLavrsen - 31 Jan 2008

My File::Temp is version 0.16 in case this is a important

-- TWiki:Main.KennethLavrsen - 31 Jan 2008

I reported Item5265. I have this issue with RevCommentPlugin. My CGI is 3.15. File system is ext3. I am on debian etch.

-- TWiki:Main.LijiYu - 01 Feb 2008

From configure:

CPAN lib Martin Kenneth
B::Deparse 0.71 installed 0.71 installed
Data::Dumper 2.121 installed 2.121 installed
FileHandle 2.01 installed 2.01 installed
File::Basename 2.74 installed 2.74 installed
File::Glob 1.05 installed 1.05 installed
File::Path 1.08 installed 1.08 installed
File::Spec 3.12 installed 3.12 installed
File::Temp 0.16 installed 0.16 installed
MIME::Base64 3.07 installed 3.07 installed
POSIX 1.09 installed 1.09 installed
Socket 1.78 installed 1.78 installed
Archive::Tar 1.30 installed 1.32 installed
CGI::Cookie 1.26 installed 1.26 installed
CGI::Session 4.14 installed 4.20 installed
Locale::Maketext::Lexicon 0.49 installed 0.49 installed
Net::SMTP 2.29 installed 2.29 installed
Apache::Htpasswd Not installed 1.8 installed
Digest::MD5 2.36 installed 2.36 installed
Digest::SHA1 2.11 installed 2.11 installed
Encode 2.12 installed 2.12 installed
Encode::compat Not installed Not installed
Getopt::Long 2.35 installed 2.35 installed
I18N::Langinfo 0.02 installed 0.02 installed
Lingua::EN::Sentence Not installed Not installed
Unicode::MapUTF8 Not installed Not installed
Win32::Console Not installed Not installed

-- TWiki:Main.MartinRowe - 01 Feb 2008

I added my CPAN lib list in a new column. I run Centos 5.

So we have same type of OS: Linux. Same file system. CPAN libs that TWiki reports are the same except Archive::Tar and CGI::Session. I doubt any of the two should have an impact in this area.

I really wonder how we can debug this further.

Martin are you able to temporarily revert the code sniplet above to see if this cures the problem?

-- TWiki:Main.KennethLavrsen - 01 Feb 2008

What about the versions of the perl installations themselves - any differences there ( perl -v)

-- TWiki:Main.SteffenPoulsen - 01 Feb 2008

"This is perl, v5.8.8 built for i386-linux-thread-multi"

-- TWiki:Main.KennethLavrsen - 01 Feb 2008

"This is perl, v5.8.8 built for i486-linux-gnu-thread-multi"

I edited Store.pm to match the diff, and attachments now upload correctly. What next wink

-- TWiki:Main.MartinRowe - 02 Feb 2008

This is perl, v5.8.8 built for i486-linux-gnu-thread-multi

-- TWiki:Main.LijiYu - 03 Feb 2008

Seems like we should revert back to the code from before 14070. CrawfordCurrie you made the 14070 change but you have been very silent on this one.

Do I just revert your change or do you want to have a go at fixing your code? This feature worked fine before the change. We never had any reports that people could not upload in 4.1.2. Now we have two independent users with very up to date Linux'es with exactly the same symptom and a confirmation that reverting the relevant part of 14070 fixes the issue. So the natural thing for me to do is to roll back to what worked.

-- TWiki:Main.KennethLavrsen - 03 Feb 2008

Most likely related to Item4611 and possibly to Item5265. I find it unfortunate that a major bug reported on 12 Sep 2007 slipped into the 4.2 release.

-- TWiki:Main.PeterThoeny - 04 Feb 2008

Yes we already referred those bug reports above in the analysis. To wrap up the related bug items

  • Item4611 - Zero file size uploads - The time it was reported matches that the error was injected with 14070
  • Item5265 - RevCommentPlugin causes zero byte attachment. - It for sure not related to the plugin but to the fact that the use of beforeAttachmentSaveHandler makes the core run exactly the piece of code that reports an error in the log.
  • Item2097 - A bug item which is related to same feature but apart of the analysis and knowledge from then I doubt it has great relevance now.
  • Item2371 - Also a bug item with information to keep in mind but again it is not the same bug that we fight now
  • Item2390 - beforeAttachmentSaveHandler is broken on Solaris and RedHat - An exact same kind of problem which was resolved. The solution from then is what was altered by 14070 later.

All in all the current best solution known is reverting code snip listed earlier in this report from the 14070 change for 4.2.1. If any new code is checked in it needs to get confirmed by both Martin and Liji.

-- TWiki:Main.KennethLavrsen - 04 Feb 2008

If you start reverting 14070, you will have to revert a huge number of other changes that followed, building on the change. So, we have to find out why this happens, and not just have a knee-jerk reaction. I have just spent the last 20 minutes trying to reproduce this problem on debian etch, and I can't, so this is something very strange.

-- TWiki:Main.CrawfordCurrie - 04 Feb 2008

Could we have a complete "installed and enabled plugins" list from the two reporters? Problem might lie in the combination of plugins somewhere.

-- TWiki:Main.SteffenPoulsen - 05 Feb 2008

Enabled plugin Version
SpreadSheetPlugin (any TWiki, $Rev: 16273 (22 Jan 2008)
BlackListPlugin (any TWiki, 13186)
CommentPlugin (TWiki-4, $Rev: 15776 (22 Jan 2008)
DateFieldPlugin (Dakar, 8083)
EditRowPlugin ($Date$, $Rev: 15989 (13 Dec 2007)
EmptyPlugin (TWiki-4.2, $Rev: 15942 (22 Jan 2008)
ImageGalleryPlugin (3.41, 11160)
InterwikiPlugin (Dakar, $Rev: 14913 (17 Sep 2007)
PreferencesPlugin (TWiki-4.2, $Rev: 15487 (22 Jan 2008)
RenderListPlugin (2.0, $Rev: 16235 (22 Jan 2008)
SlideShowPlugin (Any TWiki, $Rev: 15091 (22 Jan 2008)
SmiliesPlugin (Dakar, $Rev: 16049 (22 Jan 2008)
TablePlugin (1.032, $Rev: 16182 (22 Jan 2008)
TinyMCEPlugin (TWiki-4, $Rev: 16268 (22 Jan 2008)
TwistyPlugin (1.4.9, $Rev: 15653 (19 Nov 2007)
WysiwygPlugin (TWiki-4.2, $Rev: 16174 (22 Jan 2008)

ActionTrackerPlugin (8801) and EditTablePlugin (4.7.10 (08 Jan 2008)) are installed, but disabled in configure.

-- TWiki:Main.MartinRowe - 05 Feb 2008

This bug has been open for 7 days now. People that installed 4.2 need an urgent fix.

We had code that worked in 4.1.2. New code was added - in an unrelated bug item - which broke uploading for many.

Can we at least go back to the code that worked?

-- TWiki:Main.KennethLavrsen - 07 Feb 2008

For your info:

I have a new install of 4.2.0 on which this bug doesn't seem to manifest. I have checked that the BlackListPlugin is working, and uploaded one image file to my Sandbox successfully (just so you know I haven't tried with lots of uploads).

$ perl -e 'use CGI; print "$CGI::VERSION\n";'
3.05
$ perl -v
This is perl, v5.8.5 built for i386-linux-thread-multi

If you're keen to find out more about my config, you can email me Andy at andypryke.com and I can follow instructions to give you more info on my configuration and log this here. Good luck with nailing this one!

-- AndyPryke - 15 Feb 2008

Yes. I do not see the problem either. We have not managed to pinpoint the exact thing that triggers the problem. But we have confirmed that reverting the code as it was in 4.1.2 cures it.

It seems to me that the most plausible explanation is that the temporary file that CGI creates when you upload an attachment in some cases get deleted when the file is closed/reopened multiple times.

In the older code the file was copied to a new temporary file before it was sent to the plug-in. Unless an explanation comes up we have to revert back to that. It is more important that all customers have a TWiki that works.

-- TWiki:Main.KennethLavrsen - 20 Feb 2008

When I tested the 4.2 I had the same issue. attachments have size 0. TWiki is installed on windows xp. Here a list with the installed plugins

-- TWiki:Main.FrederikBeun - 21 Feb 2008

I have been reading up a bit on this topic.

For the moment I am working on the assumption that the CGI.pm somehow must be unlinking the temporary file in certain cases.

What is unique to installation running a plugin with beforeAttachmentSaveHandler is that the temporary file that CGI.pm creates when you upload gets closed and reopened several times. Both by the core and by the plugin via Func calls. My feeling is that somehow during this the file gets unlinked (deleted).

According to the litterature I have found CGI.pm is supposed to unlink the file when the query object goes out of scope. As far as I can see it does not. But somehow in some cases it may do anyway.

I found this note in some old docs for CGI.pm.

A potential problem with the temporary file upload feature is that the temporary file is accessible to any local user on the system. In previous versions of this module, the temporary file was world readable, meaning that anyone could peak at what was being uploaded. As of version 2.36, the modes on the temp file have been changed to read/write by owner only. Only the Web server and its CGI scripts can access the temp file. Unfortunately this means that one CGI script can spy on another! To make the temporary files really private, set the CGI global variable $CGI::PRIVATE_TEMPFILES to 1. Alternatively, call the built-in function CGI::private_tempfiles(1), or just use CGI qw/-private_tempfiles. The temp file will now be unlinked as soon as it is created, making it inaccessible to other users. The downside of this is that you will be unable to access this temporary file directly (tmpFileName() will continue to return a string, but you will find no file at that location.) Further, since PRIVATE_TEMPFILES is a global variable, its setting will affect all instances of CGI.pm if you are running mod_perl. You can work around this limitation by declaring $CGI::PRIVATE_TEMPFILES as a local at the top of your script.

Can the reporters of this bug somehow have an environment where the CGI.pm is using PRIVATE_TEMPFILES?

If there is a risk of this we should for sure go back to creating our own temporary file. We concluded in earlier bug reports that the CPAN module File::Temp is OK to use because it has been part of standard perl since 5.6.1 and this is also the oldest perl version we support. There is no unwanted CPAN dependency related to File::Temp.

How do we easily get the reporters to check for PRIVATE_TEMPFILES the easiest way?

-- TWiki:Main.KennethLavrsen - 22 Feb 2008

Good analysis Kenneth! Agreed, using File::Temp is OK.

-- TWiki:Main.PeterThoeny - 27 Feb 2008

Our customers have been waiting all too long for a fix. I have provided what I believe is a fix. Checked into both trunk and 4.2 branch. I have additionally uploaded an updated version of lib/TWiki/Store.pm so people more easily can implement a fix for this bug.

  • Store.pm: File for TWiki 4.2.0 which should fix the problem

Martin, LijiYu, and Frederik - can you please confirm that the fix is working for you? If not flip the status of this bug item back to Confirmed.

-- TWiki:Main.KennethLavrsen - 03 Mar 2008

I have encountered the same bug with the BatchUploadPlugin, I then apply the fix above (Store.pm) and that has solved the problem

-- TWiki:Main.AlainVILLOT - 03 Mar 2008

Kenneth, that version of Store.pm works for me.

-- TWiki:Main.MartinRowe - 03 Mar 2008

Excellent, thank you very much Kenneth for fixing this bug!

-- TWiki:Main.PeterThoeny - 04 Mar 2008

It works with RevCommentPlugin now. Thanks a lot!

-- TWiki:Main.LijiYu - 04 Mar 2008

OK. Confirmed by all reporters so it was the right to do. Customer first.

-- TWiki:Main.KennethLavrsen - 06 Mar 2008

ItemTemplate
Summary Plugins with beforeAttachmentSaveHandler break file attachments with TWiki 4.2.0
ReportedBy TWiki:Main.MartinRowe
Codebase 4.2.0, ~twiki4
SVN Range TWiki-5.0.0, Wed, 23 Jan 2008, build 16283
AppliesTo Engine
Component

Priority Urgent
CurrentState Closed
WaitingFor

Checkins TWikirev:16478 TWikirev:16479
TargetRelease patch
ReleasedIn 4.2.1, 5.0.0
Topic attachments
I Attachment History Action Size Date Who Comment
Perl source code filepm Store.pm r1 manage 60.0 K 2008-03-03 - 02:51 UnknownUser File for TWiki 4.2.0 which should fix the problem
PNGpng plugin_List.png r1 manage 44.7 K 2008-02-21 - 14:56 UnknownUser Plugin List FrederikBeun
Edit | Attach | Watch | Print version | History: r35 < r34 < r33 < r32 < r31 | Backlinks | Raw View |  Raw edit | More topic actions
Topic revision: r35 - 2008-08-04 - KennethLavrsen
 
This site is powered by the TWiki collaboration platform Powered by PerlCopyright © 2008-2024 by the contributing authors. All material on this collaboration platform is the property of the contributing authors.
Ideas, requests, problems regarding TWiki? Send feedback