You are viewing a plain text version of this content. The canonical link for it is here.
Posted to apreq-dev@httpd.apache.org by Markus Wichitill <ma...@gmx.de> on 2004/07/22 15:13:23 UTC
[apreq-2] Open upload with or without :APR
We still need to decide when to use :APR and when not in $upload->fh, and
when to tell users to use :APR for $upload->tempname and $upload->link.
After re-reading the related thread
(http://article.gmane.org/gmane.comp.apache.apreq/2010), I think we only
actually need :APR on Windows, right? We don't need to care about more
complicated has_large_files_conflict() stuff on Unix, since :APR is only
needed to set those two special Win32 file flags.
So how about these simple "if ($^O =~ /Win32/)" patches?
Index: glue/perl/xsbuilder/Apache/Upload/Upload_pm
===================================================================
RCS file:
/home/cvspublic/httpd-apreq-2/glue/perl/xsbuilder/Apache/Upload/Upload_pm,v
retrieving revision 1.10
diff -u -r1.10 Upload_pm
--- glue/perl/xsbuilder/Apache/Upload/Upload_pm 8 Jul 2004 19:55:57 -0000 1.10
+++ glue/perl/xsbuilder/Apache/Upload/Upload_pm 22 Jul 2004 10:53:48 -0000
@@ -14,8 +14,15 @@
sub fh {
my $upload = shift;
- open my $fh, "<:APR", $upload->tempname, $upload->pool or
- die "Can't open ", $upload->tempname, ": ", $!;
+ my $fh;
+ if ($^O =~ /Win32/) {
+ open $fh, "<:APR", $upload->tempname, $upload->pool
+ or die "Can't open ", $upload->tempname, ": ", $!;
+ }
+ else {
+ open $fh, "<", $upload->tempname
+ or die "Can't open ", $upload->tempname, ": ", $!;
+ }
return $fh;
}
Index: glue/perl/t/response/TestApReq/upload.pm
===================================================================
RCS file:
/home/cvspublic/httpd-apreq-2/glue/perl/t/response/TestApReq/upload.pm,v
retrieving revision 1.1
diff -u -r1.1 upload.pm
--- glue/perl/t/response/TestApReq/upload.pm 12 Jul 2004 13:10:14 -0000 1.1
+++ glue/perl/t/response/TestApReq/upload.pm 22 Jul 2004 11:07:37 -0000
@@ -31,8 +31,14 @@
}
elsif ($method eq 'tempname') {
my $name = $upload->tempname;
- open $fh, "<:APR", $name, $upload->pool
- or die "Can't open $name: $!";
+ if ($^O =~ /Win32/) {
+ open $fh, "<:APR", $name, $upload->pool
+ or die "Can't open $name: $!";
+ }
+ else {
+ open $fh, "<", $name
+ or die "Can't open $name: $!";
+ }
binmode $fh;
read $fh, $data, $upload->size;
close $fh;
@@ -41,7 +47,14 @@
my $link_file = File::Spec->catfile($temp_dir, "linkfile");
unlink $link_file if -f $link_file;
$upload->link($link_file) or die "Can't link to $link_file: $!";
- open $fh, "<", $link_file or die "Can't open $link_file: $!";
+ if ($^O =~ /Win32/) {
+ open $fh, "<:APR", $link_file, $upload->pool
+ or die "Can't open $link_file: $!";
+ }
+ else {
+ open $fh, "<", $link_file
+ or die "Can't open $link_file: $!";
+ }
binmode $fh;
read $fh, $data, $upload->size;
close $fh;
Index: glue/perl/t/response/TestApReq/request.pm
===================================================================
RCS file:
/home/cvspublic/httpd-apreq-2/glue/perl/t/response/TestApReq/request.pm,v
retrieving revision 1.33
diff -u -r1.33 request.pm
--- glue/perl/t/response/TestApReq/request.pm 18 Jul 2004 23:24:51 -0000 1.33
+++ glue/perl/t/response/TestApReq/request.pm 22 Jul 2004 11:49:29 -0000
@@ -72,8 +73,15 @@
chop $dir;
die "Tempfile in wrong temp_dir (expected $temp_dir, saw $dir)" unless
$dir eq $temp_dir;
-
- open my $fh, "<:APR", $name, $upload->pool or die "Can't open
$name: $!";
+ my $fh;
+ if ($^O =~ /Win32/) {
+ open $fh, "<:APR", $name, $upload->pool
+ or die "Can't open $name: $!";
+ }
+ else {
+ open $fh, "<", $name
+ or die "Can't open $name: $!";
+ }
$r->print(<$fh>);
}
elsif ($test eq 'link') {
@@ -81,7 +89,15 @@
my $link_file = File::Spec->catfile("$temp_dir", "linktest");
unlink $link_file if -f $link_file;
$upload->link($link_file) or die "Can't link to $link_file: $!";
- open my $fh, "<", $link_file or die "Can't open $link_file: $!";
+ my $fh;
+ if ($^O =~ /Win32/) {
+ open $fh, "<:APR", $link_file, $upload->pool
+ or die "Can't open $link_file: $!";
+ }
+ else {
+ open $fh, "<", $link_file
+ or die "Can't open $link_file: $!";
+ }
$r->print(<$fh>);
}
elsif ($test eq 'fh') {
Regarding the last chunk: for the link test in request.pm to succeed, I
didn't actually need the :APR on Win32, unlike for the link test in
upload.pm. I'm not sure why, but I guess using the same code can't hurt.
On Win32 Perl 5.8 is required anyway and the AS binaries used by most people
have PerlIO enabled. Both the AS binaries and the official Apache binaries
have largefiles support. So unless someone compiles Perl without PerlIO or
Perl and Apache with diverging largefile support, Win32 users should be able
to use :APR.
Of course it's still not possible to simply pass $upload->tempname to
someone's Perl module or external program (which is was what the person that
requested tempname on the mp list wanted) and be Win32-compatible.
Re: [apreq-2] Open upload with or without :APR
Posted by Randy Kobes <ra...@theoryx5.uwinnipeg.ca>.
On Fri, 23 Jul 2004, Markus Wichitill wrote:
> Joe Schaefer wrote:
> >>All in all, this is so ugly and confusing that I still think that
> >>explicitely deleting the tempfile with a cleanup handler without using
> >>special Win32 flags would be the simplest solution,
> >
> > +1 (bigtime). We need to remove APR_DELONCLOSE in apreq_file_mktemp and
> > have it register a pool cleanup to delete the created file.
>
> Thanks for making this change. Tests ok on Linux, on Win32 $upload->fh needs
> binmode.
>
> The attached patch adds that, removes :APR and removes the FAQ entry. It
> also contains the server_root_relative change, I think that one also works
> with slightly older mp2.
Thanks! That's now been applied in cvs.
--
best regards,
randy
Re: [apreq-2] Open upload with or without :APR
Posted by Markus Wichitill <ma...@gmx.de>.
Joe Schaefer wrote:
>>All in all, this is so ugly and confusing that I still think that
>>explicitely deleting the tempfile with a cleanup handler without using
>>special Win32 flags would be the simplest solution,
>
> +1 (bigtime). We need to remove APR_DELONCLOSE in apreq_file_mktemp and
> have it register a pool cleanup to delete the created file.
Thanks for making this change. Tests ok on Linux, on Win32 $upload->fh needs
binmode.
The attached patch adds that, removes :APR and removes the FAQ entry. It
also contains the server_root_relative change, I think that one also works
with slightly older mp2.
Re: [apreq-2] Open upload with or without :APR
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Markus Wichitill <ma...@gmx.de> writes:
> Joe Schaefer wrote:
> > Markus Wichitill <ma...@gmx.de> writes:
> >>Regarding the last chunk: for the link test in request.pm to succeed,
> >>I didn't actually need the :APR on Win32, unlike for the link test in
> >>upload.pm. I'm not sure why, but I guess using the same code can't hurt.
> > Probably because link()ed files don't need to SHARE_DELETE,
> > since only the temp file is actually scheduled for deletion.
So I have this completely wrong then, big surprise :-).
> But the link test in upload.pm does need the :APR open in my case, since
> otherwise opening the file will fail with "permission denied". That's
> what I reported in my various issues thread.
Ok, now it is clearer to me- thanks. What was confusing was that
the link() test code in request.pm was passing there, but not for
upload.pm. I believe the reason for this is that in request.pm
the upload brigade is entirely in-memory (it's roughly 100 KB in size,
which is < 256 KB required to force a write to disk). The write-to-disk
actually happens during link() (see Apache__Upload.h line 148), in which
case we do *not* pass the DELONCLOSE flag. That explains why you don't
currently need ":<APR" for smallish uploads.
[...]
> All in all, this is so ugly and confusing that I still think that
> explicitely deleting the tempfile with a cleanup handler without using
> special Win32 flags would be the simplest solution,
+1 (bigtime). We need to remove APR_DELONCLOSE in apreq_file_mktemp and
have it register a pool cleanup to delete the created file.
--
Joe Schaefer
Re: [apreq-2] Open upload with or without :APR
Posted by Markus Wichitill <ma...@gmx.de>.
Joe Schaefer wrote:
> Markus Wichitill <ma...@gmx.de> writes:
>>Regarding the last chunk: for the link test in request.pm to succeed,
>>I didn't actually need the :APR on Win32, unlike for the link test in
>>upload.pm. I'm not sure why, but I guess using the same code can't hurt.
>
> Probably because link()ed files don't need to SHARE_DELETE,
> since only the temp file is actually scheduled for deletion.
But the link test in upload.pm does need the :APR open in my case, since
otherwise opening the file will fail with "permission denied". That's what I
reported in my various issues thread.
SysInternals FileMon is more specific and reports a "SHARING VIOLATION":
Apache.exe:1548 OPEN C:\DOKUME~1\mawic\Temp\apreqK3PHF7 SUCCESS Options:
Open Access: All
Apache.exe:1548 QUERY INFORMATION C:\DOKUME~1\mawic\Temp\apreqK3PHF7
SUCCESS Attributes: A
Apache.exe:1548 OPEN C:\DOKUME~1\mawic\Temp\linkfile SUCCESS Options:
Open Access: All
Apache.exe:1548 SET INFORMATION C:\DOKUME~1\mawic\Temp\apreqK3PHF7
SUCCESS FileLinkInformation
Apache.exe:1548 CLOSE C:\DOKUME~1\mawic\Temp\linkfile SUCCESS
Apache.exe:1548 CLOSE C:\DOKUME~1\mawic\Temp\apreqK3PHF7 SUCCESS
Apache.exe:1548 OPEN C:\DOKUME~1\mawic\Temp\linkfile SHARING VIOLATION
Options: Open Access: All
The "SET INFORMATION FileLinkInformation" line confirms that a real NTFS
hardlink is created, and not a copy.
Randy, can you confirm that the upload.pm link test doesn't need :APR in
your case because $upload->link creates a copy for you?
What still somewhat confuses me is that I get this sharing violation,
whereas Randy's non-APR opening of the original tempfile nukes it, according
to his old thread. Of course that might just be the difference between
original hardlink and new hardlink.
>>Of course it's still not possible to simply pass $upload->tempname to
>>someone's Perl module or external program (which is was what the
>>person that requested tempname on the mp list wanted) and be
>>Win32-compatible.
>
> We're about as close (with your patch, e.g.) as we can reasonably go with
> this, IMO. We want to allow folks to pass the name to an external app for
> processing, but it's not our problem if *that* app has trouble
> opening the file.
Well, we effectively lock the file by using those special file flags.
> We just need to remind module authors about the
> usefulness of link() in this context,
Unless I'm completely wrong with my diagnosis above, link isn't more useful
than tempname in this context.
> and that fh() (or better io()),
> also work as documented on all supported platforms. If so, we really
> just need fh() to use "<:APR" on Win32, and document that fh() requires
> 5.8 on Windows.
mod_perl 2.0 already requires Perl 5.8 on Windows because of the threading,
so this would only be relevant for CGI I guess. However what needs to be
documented is that on Windows:
- $upload->tempname requires :APR
- Hardlinks created by $upload->link require :APR when they're opened during
the same request (unless they're created on a different partition or on a
FAT partition)
- If the user wants to pass a filename to an external program, he has to
create a copy of the upload data by writing $upload->bb or $upload->io to a
new file
All in all, this is so ugly and confusing that I still think that
explicitely deleting the tempfile with a cleanup handler without using
special Win32 flags would be the simplest solution, especially if that's
what APR already does on Unix. The occasional leftover tempfile can be
cleaned up by a cronjob, and there are simpler DoS attacks than uploading
files while hoping that enough of them stick around because of segfaults etc
to fill a modern HD.
Re: [apreq-2] Open upload with or without :APR
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Markus Wichitill <ma...@gmx.de> writes:
[...]
> I think we only actually need :APR on Windows, right? We don't need to
> care about more complicated has_large_files_conflict() stuff on Unix,
> since :APR is only needed to set those two special Win32 file flags.
>
> So how about these simple "if ($^O =~ /Win32/)" patches?
Looks ok to me in principle- but this is really Randy's call.
[...]
> Regarding the last chunk: for the link test in request.pm to succeed,
> I didn't actually need the :APR on Win32, unlike for the link test in
> upload.pm. I'm not sure why, but I guess using the same code can't hurt.
Probably because link()ed files don't need to SHARE_DELETE,
since only the temp file is actually scheduled for deletion.
> On Win32 Perl 5.8 is required anyway and the AS binaries used by most
> people have PerlIO enabled. Both the AS binaries and the official
> Apache binaries have largefiles support. So unless someone compiles
> Perl without PerlIO or Perl and Apache with diverging largefile
> support, Win32 users should be able to use :APR.
This is good to know- thanks!
> Of course it's still not possible to simply pass $upload->tempname to
> someone's Perl module or external program (which is was what the
> person that requested tempname on the mp list wanted) and be
> Win32-compatible.
We're about as close (with your patch, e.g.) as we can reasonably go with
this, IMO. We want to allow folks to pass the name to an external app for
processing, but it's not our problem if *that* app has trouble
opening the file. We just need to remind module authors about the
usefulness of link() in this context, and that fh() (or better io()),
also work as documented on all supported platforms. If so, we really
just need fh() to use "<:APR" on Win32, and document that fh() requires
5.8 on Windows.
If someone sorts this Win32 tempfile mess out, please patch FAQ.pod.
Thanks.
--
Joe Schaefer