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?
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