Tags:
create new tag
view all tags

Bug: Upload uses Undocumented CGI Function

I tagged it as a CODE_SMELL in the codebase, but I realised it's actually a bit smellier than that so I'm raising a bug here.

TWiki::UI::Upload::upload uses an undocumented CGI function, tmpFileName, to recover the name of the file that Apache creates during an upload. On the face of it this is not so bad, but it means that using TWiki::UI::Upload::upload is rather perilous for the unwary - use it wrong, and CGI exhibits very mysterious behaviours (which is how I found it in the first place).

The reason the method is used is to recover the server-side file name of the uploaded temporary file. The reason why the local file is wanted, rather than a filehandle to the local file which is available from CGI, is so that a stat can be performed on the file to determine it's size.

IMHO the CGI API should be respected, same as the core expects Plugins authors to respect the TWiki::Func API.

There is a simple fix for this, and that is to pipe the file into a temporary file that TWiki actually owns. The performance hit of doing this is minimal, unless the file is mega-big. This seems safer to me than risking the use of an undocumented function. The alternative is a documentation fix that warns people not to use TWiki::UI::Upload::upload (they should be using TWiki::UI::Upload::updateAttachment anyway). I'd be quite happy to provide a patch for either fix, if so requested.

-- CrawfordCurrie - 18 May 2004

Follow up

Hmm, legacy. The original code was taken from somewhere else. Better to fix properly. Does the proposed fix work also for 100 MB attachments?

-- PeterThoeny - 20 May 2004

As I said, "unless the file is mega-big". Does anything work for 100Mb attachments? hehe!

The most robust, most "Apache friendly", most long-term solution is to pass the filehandle into the functions called by upload, instead of the name of the temporary file. stat can be used on a filehandle, so everything done presently on the temporary file can still be done. We would have to pass around the filehandle instead of the file name, which would touch four or five places. Of these, the scariest is Store.pm, where it would have to go into at least one of those mysterious undocumented functions. But it's do-able, with care.

-- CrawfordCurrie - 20 May 2004

Yes, we have big attachments. At work we have folks who upload CD images...

-- PeterThoeny - 20 May 2004

Fix record

Edit | Attach | Watch | Print version | History: r4 < r3 < r2 < r1 | Backlinks | Raw View | Raw edit | More topic actions
Topic revision: r4 - 2004-07-11 - PeterThoeny
 
  • Learn about TWiki  
  • Download TWiki
This site is powered by the TWiki collaboration platform Powered by Perl Hosted by OICcam.com Ideas, requests, problems regarding TWiki? Send feedback. Ask community in the support forum.
Copyright © 1999-2024 by the contributing authors. All material on this collaboration platform is the property of the contributing authors.