Bug #52469 for CGI: PUT method with empty body seems to freeze CGI->new()

Sun Dec 06 06:22:31 2009 philip.g.potter [...] gmail.com - Ticket created

Subject:

PUT method with empty body seems to freeze CGI->new()

A PUT HTTP request with no body and no Content-Length or Content-Type headers seems to freeze up CGI->new(). I found this while trying to install Test::WWW::Mechanize, whose t/put_ok.t script fails under CGI versions 3.44, 3.45, and 3.48. 3.43 works fine, as does 3.29; I haven't tested any others. Steps to reproduce: 1. install CGI 3.44 or later 2. get Test::WWW::Mechanize (I'm using 1.24 but I don't think it matters) 3. unpack T::WWW::M 4. in the T::WWW::M directory, call "perl Makefile.PL" and "make" to set up the blib/ directory 5. in the T::WWW::M directory, call "prove -bv t/put_ok.t". The HTTP PUT requests will time out. I have narrowed the path of execution down to this: * T::WWW::M's test t/put_ok.t calls the testserver in t/TestServer.pm * TestServer subclasses HTTP::Server::Simple::CGI * The method HTTP::Server::Simple::CGI::handler calls CGI->new() * CGI->new() calls CGI->init() * CGI->init() calls CGI->read_from_stdin() around line 651 * CGI->read_from_stdin() never returns It's difficult to say that this is invalid behaviour, since the utility of an empty PUT request is questionable. However there's nothing in the RFC I can see which explicitly forbids an empty PUT request, and if there's no Content-Length header then it's certainly possible that there's no body to the HTTP request. In any case, even if you think an empty HTTP PUT request is invalid, CGI should probably fail gracefully rather than getting stuck. I had previously raised this issue with T::WWW::M before finding out that CGI is probably responsible. The previous discussion can be viewed here: http://code.google.com/p/www-mechanize/issues/detail?id=127

Sun Dec 06 12:02:06 2009 yanick [...] babyl.dyndns.org - Correspondence added

Subject:	Re: [rt.cpan.org #52469] PUT method with empty body seems to freeze CGI->new()
Date:	Sun, 06 Dec 2009 12:04:54 -0500
To:	bug-CGI.pm [...] rt.cpan.org
From:	Yanick Champoux <yanick [...] babyl.dyndns.org>

Philip Potter via RT wrote: Show quoted text

> A PUT HTTP request with no body and no Content-Length or Content-Type > headers seems to freeze up CGI->new().

Here's a minimal test that reproduces the problem: use CGI; $ENV{REQUEST_METHOD} = 'PUT'; my $cgi = CGI->new; Yup, that's it. Told ya it was minimal. :-) `/anick

Sun Dec 06 12:02:07 2009 The RT System itself - Status changed from 'new' to 'open'

Sun Dec 06 12:47:24 2009 yanick [...] babyl.dyndns.org - Correspondence added

Subject:	Re: [rt.cpan.org #52469] PUT method with empty body seems to freeze CGI->new()
Date:	Sun, 06 Dec 2009 12:50:27 -0500
To:	bug-CGI.pm [...] rt.cpan.org
From:	Yanick Champoux <yanick [...] babyl.dyndns.org>

ah AH! Got the culprit! Commit 11a14db3b849388a in the Git repo. Patch made the 14th of May 2009, with the comment 'Patch from Kurt Jaeger to allow HTTP PUT even if the content length is unknown.'

Sun Dec 06 14:32:47 2009 yanick [...] babyl.dyndns.org - Correspondence added

Subject:	Re: [rt.cpan.org #52469] PUT method with empty body seems to freeze CGI->new()
Date:	Sun, 06 Dec 2009 14:35:36 -0500
To:	bug-CGI.pm [...] rt.cpan.org
From:	Yanick Champoux <yanick [...] babyl.dyndns.org>

Potential fix at http://github.com/yanick/CGI.pm/commits/rt-52469 I inserted a timeout of 5 seconds for reading a line from STDIN. After that time, if we got nothing we bail out of the read_from_stdin function. I'm trying to remember if there's a more elegant way to verify that something is coming down the STDIN pipe, but I can't think of anything right now...

Sun Dec 06 14:43:01 2009 philip.g.potter [...] gmail.com - Correspondence added

On Sun Dec 06 14:32:47 2009, yanick@babyl.dyndns.org wrote: Show quoted text

> I'm trying to remember if there's a more elegant way to verify that > something is coming down the STDIN pipe, but I can't think of anything > right now...

It's certainly not ideal; it's trying to second-guess the user's behaviour when they haven't given us enough information to work out what they're doing. It's like trying to work out the user's charset when they haven't told you; really, you just want to tell them to go away until they present their data properly. Idealistically, I would say if the user doesn't put a Content-Length: header then they can't expect any content to be used. Are there user agents in the wild which make PUT requests without setting Content-Length? Clearly if there are a significant number then they should be supported; but if not (and given how many people make PUT requests in the first place, I suspect not) then is this something that needs to be supported? Was the original change to support this made in response to user demand? Does it have an attached ticket?

Wed Dec 09 21:42:19 2009 MARKSTOS [...] cpan.org - Taken

Wed Dec 09 21:46:50 2009 MARKSTOS [...] cpan.org - Correspondence added

Thanks for the report. To consider this, I read some of the HTTP RFC 2616 myself, as well as the CGI RFC 3875: http://www.ietf.org/rfc/rfc3875 In particular, there's this: "The server MUST set this meta-variable if and only if the request is accompanied by a message-body entity." I also commented on the issue via github: http://github.com/yanick/CGI.pm/commit/b132e1a6676b1698b15b1ece0944ac620 4e17fc7 I think the CGI::Simple approach closely models the spec, and we consider that approach: If a content_length is set, it reads that many bytes. If no content_length is set, it reads no bytes. I read the HTTP spec first, and got on detour reading about "Tranfer- Encoding" and chunked messages, but the CGI spec seems rather clear on the matter. Yanick, would you mind trying a second patch that more closely follows the CGI::Simple approach? Mark

Wed Dec 09 21:55:30 2009 MARKSTOS [...] cpan.org - Reference to ticket #51109 added

Thu Dec 10 05:22:14 2009 philip.g.potter [...] gmail.com - Correspondence added

Hi Mark, Thanks for your message. I largely agree with your approach here. The CGI::Simple approach seems correct to me. Second guessing the user agent just seems like trouble.

Thu Dec 10 12:18:30 2009 yanick.champoux [...] gmail.com - Correspondence added

Subject:	Re: [rt.cpan.org #52469] PUT method with empty body seems to freeze CGI->new()
Date:	Thu, 10 Dec 2009 12:18:00 -0500
To:	bug-CGI.pm [...] rt.cpan.org
From:	Yanick Champoux <yanick.champoux [...] gmail.com>

On December 9, 2009 09:46:52 pm MARKSTOS via RT wrote: Show quoted text

> Yanick, would you mind trying a second patch that more closely follows > the CGI::Simple approach?

No problem at all, will do. :-) `/anick

Thu Dec 16 21:34:13 2010 MARKSTOS [...] cpan.org - Correspondence added

RT-Send-CC:

yanick.champoux [...] gmail.com

Yanick or Philip, of you could submit a refined patch here based on the CGI::Simple code, that would be great.

Sat Dec 18 12:49:53 2010 yanick [...] babyl.dyndns.org - Correspondence added

Subject:	Re: [rt.cpan.org #52469] PUT method with empty body seems to freeze CGI->new()
Date:	Sat, 18 Dec 2010 12:50:32 -0500
To:	bug-CGI.pm [...] rt.cpan.org
From:	Yanick Champoux <yanick [...] babyl.dyndns.org>

Show quoted text

> Yanick or Philip, of you could submit a refined patch here based on the > CGI::Simple code, that would be great.

No problem. I'll try to do that tonight. Joy, `/anick

Tue Dec 21 12:12:35 2010 yanick [...] babyl.dyndns.org - Correspondence added

Subject:	Re: [rt.cpan.org #52469] PUT method with empty body seems to freeze CGI->new()
Date:	Tue, 21 Dec 2010 12:13:24 -0500
To:	bug-CGI.pm [...] rt.cpan.org
From:	Yanick Champoux <yanick [...] babyl.dyndns.org>

Looking at the code, and if we go the way of not reading anything if CONTENT_LENGTH is not defined, I think we'll interfere with the stanza of line 666 of CGI.pm: elsif (not defined $ENV{CONTENT_LENGTH}) { $self->read_from_stdin(\$query_string); # should this be PUTDATA in case of PUT ? my($param) = $meth . 'DATA' ; $self->add_parameter($param) ; push (@{$self->{param}{$param}},$query_string); undef $query_string ; } If I'm not mistaken, this is probably there to take care of the case where the script is run from the command-line. On the plus side, though, this seems to be the only place where read_from_stdin is used, so if we go ahead with the change, at least the effects of it will be contained.

Fri Dec 24 17:20:22 2010 MARKSTOS [...] cpan.org - Correspondence added

On Tue Dec 21 12:12:35 2010, yanick@babyl.dyndns.org wrote: Show quoted text

> > Looking at the code, and if we go the way of not reading anything if > CONTENT_LENGTH is not defined, I think we'll interfere with the stanza > of line 666 of CGI.pm: > > elsif (not defined $ENV{CONTENT_LENGTH}) { > $self->read_from_stdin(\$query_string); > # should this be PUTDATA in case of PUT ? > my($param) = $meth . 'DATA' ; > $self->add_parameter($param) ; > push (@{$self->{param}{$param}},$query_string); > undef $query_string ; > } > > > If I'm not mistaken, this is probably there to take care of the case > where the script is run from the command-line. On the plus side,

though, Show quoted text

> this seems to be the only place where read_from_stdin is used, so if

we Show quoted text

> go ahead with the change, at least the effects of it will be

contained. The read_from_stdin() method was only introduced in 3.44... in the patch you tracked down entitled "Patch from Kurt Jaeger to allow HTTP PUT even if the content length is unknown." I think the impact will be contained to changing the affect of that patch, which seems like it was essentially the source of this bug.

Sat Dec 25 15:42:35 2010 MARKSTOS [...] cpan.org - Correspondence added

Giving to Yanick, who I think has a patch-in-progress for this. Mark

Fri Nov 11 20:40:03 2011 MARKSTOS [...] cpan.org - Told changed from Sat Dec 25 15:42:36 2010 to Fri Nov 11 20:40:03 2011

Fri Nov 11 22:39:16 2011 MARKSTOS [...] cpan.org - Correspondence added

RT-Send-CC:

yanick.champoux [...] gmail.com

Since It's been about a year, I think it's safe to assume that Yanick won't be getting to this. A patch from someone else is welcome. Ideally, it will be in the form of a github pull request that I can easily merge.

Sat Nov 12 13:09:35 2011 yanick+cpan [...] babyl.dyndns.org - Correspondence added

On Fri Nov 11 22:39:16 2011, MARKSTOS wrote: Show quoted text

> Since It's been about a year, I think it's safe to assume that Yanick > won't be getting to this.

Apologies for that. As usual, I've been pulled in many directions, and I'm afraid my CGI.pm patches fell on the wayside. I've dusted off that particular patch, and submitted a github pull off it: https://github.com/markstos/CGI.pm/pull/7

Tue Nov 15 10:48:23 2011 mark [...] summersault.com - Correspondence added

Subject:	Re: [rt.cpan.org #52469] PUT method with empty body seems to freeze CGI->new()
Date:	Tue, 15 Nov 2011 10:47:55 -0500
To:	bug-cgi.pm [...] rt.cpan.org
From:	Mark Stosberg <mark [...] summersault.com>

Show quoted text

> I've dusted off that particular patch, and submitted a github pull off it: > > https://github.com/markstos/CGI.pm/pull/7

Awesome. Thanks, Yanick.

Tue Mar 27 04:39:41 2012 complx-on-cpan [...] opsec.eu - Correspondence added

I provided the patch to allow CGI.pm read from stdin even if no content-length-header was sent. There's a scanner available from AVISION which allows HTTP PUT to submit scanner input. The system does not provide a content-lenght-header. I'm trying to get them fix this, for a while now...

Tue Mar 27 08:42:20 2012 mark [...] summersault.com - Correspondence added

Subject:	Re: [rt.cpan.org #52469] PUT method with empty body seems to freeze CGI->new()
Date:	Tue, 27 Mar 2012 08:41:45 -0400
To:	bug-cgi.pm [...] rt.cpan.org
From:	Mark Stosberg <mark [...] summersault.com>

Thanks for the nudge on this ticket.

Thu May 22 08:06:07 2014 LEEJO [...] cpan.org - Correspondence added

This issue has been copied to: https://github.com/leejo/CGI.pm/issues/63 please take all future correspondence there. This ticket will remain open but please do not reply here. This ticket will be closed when the github issue is dealt with.

Fri May 23 14:28:05 2014 The RT System itself - Queue changed from CGI.pm to CGI

Fri Jun 27 16:05:30 2014 LEEJO [...] cpan.org - Correspondence added

It seems this issue has been resolved by the original pull request merge above, d8277a5. The original patch to accept a PUT request with no content length didn't make sense and was to support a client not doing the right thing, the original changes have effectively been reverted fixing this issue. Closing.

Fri Jun 27 16:05:31 2014 LEEJO [...] cpan.org - Status changed from 'open' to 'resolved'

Fri Jun 27 16:05:32 2014 LEEJO [...] cpan.org - Fixed in 3.59 added

Bug #52469 for CGI: PUT method with empty body seems to freeze CGI->new()

Preferred bug tracker