Last Comment Bug 513989 - large search query causing internal server error (500) but valid redirect 302 returned
: large search query causing internal server error (500) but valid redirect 302...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Bugzilla 3.4
Assigned To: David Lawrence [:dkl]
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-01 12:58 PDT by Robert Elves
Modified: 2010-03-05 10:52 PST (History)
6 users (show)
mkanat: approval+
mkanat: approval3.6+
mkanat: approval3.4+
mkanat: blocking3.4.6+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wget debug output (33.76 KB, text/plain)
2009-09-04 00:19 PDT, Steffen Pingel
no flags Details
the post request (7.99 KB, text/plain)
2009-09-04 00:20 PDT, Steffen Pingel
no flags Details
Patch to not redirect in buglist.cgi if URI is too long for 3.4 (v1) (1.42 KB, patch)
2010-03-01 15:08 PST, David Lawrence [:dkl]
mkanat: review-
Details | Diff | Splinter Review
Patch to not redirect in buglist.cgi if URI is too long for 3.6 (v1) (1.66 KB, patch)
2010-03-01 15:09 PST, David Lawrence [:dkl]
mkanat: review-
Details | Diff | Splinter Review
Patch to not redirect in buglist.cgi if URI is too long for 3.4/3.6 (v2) (1.36 KB, patch)
2010-03-02 08:46 PST, David Lawrence [:dkl]
mkanat: review+
Details | Diff | Splinter Review

Description Robert Elves 2009-09-01 12:58:56 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: 3.4.1

When a search is posted, the code introduced on bug#15809 cleans the url and causes a redirect. In the event that the number of parameters in the search is very large, an internal server error occurs but instead of returning the error as a 500 response, the repository proceeds to send back a 302 redirect.  In this scenario where the query has a large number of parameters, the redirect header is a truncated (8k max) resulting in a non-deterministic query result.

This is problematic for the Mylyn Bugzilla connector since it can lead to hundreds of false incoming notifications when the redirect doesn't contain the date range qualifier (i.e. all tasks get returned instead of only those that have changed within the range specified in the original post).

Reproducible: Sometimes

Steps to Reproduce:
1.Set up a large query in Mylyn (i.e. > 1000 bugs)
2.Synchronize all tasks marking read
3.Re-synchronize all tasks, eventually this problem will occur and results in numerous incomings
Comment 1 Max Kanat-Alexander 2009-09-02 15:09:22 PDT
Um, that's very strange, because it's Apache that determines the maximum URL length. Are you saying that Apache is cutting off the actual Location header being returned?
Comment 2 Steffen Pingel 2009-09-04 00:19:42 PDT
Created attachment 398601 [details]
wget debug output

The attachment shows the output of the following command:

wget --debug -O out.txt --post-file=post.txt http://mylyn.eclipse.org/bugs34/buglist.cgi &> request.txt
Comment 3 Steffen Pingel 2009-09-04 00:20:32 PDT
Created attachment 398602 [details]
the post request
Comment 4 Steffen Pingel 2009-09-04 00:30:22 PDT
(In reply to comment #1)
> Um, that's very strange, because it's Apache that determines the maximum URL
> length. Are you saying that Apache is cutting off the actual Location header
> being returned?

Yes, that is exactly what is happening. Below is a dump of a request that results in a redirect larger than 8kb. Buglist.cgi returns a 302 but Apache detects an error, appends the output for a 500 error and writes this to the log:

[Fri Sep 04 03:20:58 2009] [error] [client 24.80.3.22] malformed header from script. Bad header=2C132602%2C114124%2C132608%2C1: buglist.cgi

Mylyn follows the valid looking redirect which results in a different being executed than originally requested.

Wireshark dump (part of the URL truncated with [...]):

POST /bugs34/buglist.cgi HTTP/1.0
User-Agent: Wget/1.11.4
Accept: */*
Host: mylyn.eclipse.org
Connection: Keep-Alive
Content-Type: application/x-www-form-urlencoded
Content-Length: 8185

query_format=advanced&chfieldfrom=2009-08-31+15%3A03%3A32+EDT&chfieldto=Now&bug_id=247911%2C247912%2C228377%2C210433%2C186044%2C192558[...]


HTTP/1.1 302 Found
Date: Fri, 04 Sep 2009 07:20:57 GMT
Server: Apache/2.2.0 (Linux/SUSE)
Location: http://mylyn.eclipse.org/bugs34/buglist.cgi?ctype=rdf&chfieldto=Now&query_format=advanced&chfieldfrom=2009-08-31%2015%3A03%3A32%20EDT&bug_id=247911%2C247912%2C2
[...]
2C277999%2C173121%2C188524%2C106862%2C188523%2C106861%2C247900%2C277991%2C247901%2C247903%2C255948%2C192544%2C186011%
Content-Length: 0
Connection: close
Content-Type: application/x-cgi

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>302 Found</title>
</head><body>
<h1>Found</h1>
<p>The server encountered an internal error or
misconfiguration and was unable to complete
your request.</p>
<p>Please contact the server administrator,
 [no address given] and inform them of the time the error occurred,
and anything you might have done that may have
caused the error.</p>
<p>More information about this error may be available
in the server error log.</p>
<p>Additionally, a 500 Internal Server Error
error was encountered while trying to use an ErrorDocument to handle the request.</p>
<hr>
<address>Apache/2.2.0 (Linux/SUSE) Server at mylyn.eclipse.org Port 80</address>
</body></html>
Comment 5 Denis Roy 2009-09-04 06:59:04 PDT
> [Fri Sep 04 03:20:58 2009] [error] [client 24.80.3.22] malformed header from
> script. Bad header=2C132602%2C114124%2C132608%2C1: buglist.cgi


Since the upgrade to 3.4.1 I've been seeing frequent errors like that one in my error_log.
Comment 6 Max Kanat-Alexander 2009-09-04 13:22:29 PDT
Interesting. Indeed, the problem is that the entire response header is greater than Apache's limit of 8K--quite a rare situation indeed.

The solution here is for us to simply not do the redirect when the resulting URL is that large, and instead just display the results during the POST.
Comment 7 David Lawrence [:dkl] 2010-03-01 09:34:17 PST
We are having issues with this as well with people submitting long queries using POST. They assume that using POST with buglist.cgi will solve the URI lenght limitation we have set on out webapps but since buglist.cgi redirects to GET then it still fails. 

Is it possible to have buglist.cgi not do the redirect if $ENV{CONTENT_LENGTH} is greater than some value?

Dave
Comment 8 Max Kanat-Alexander 2010-03-01 12:52:31 PST
@Dave It is indeed possible, and I'd accept a patch that does that.
Comment 9 Max Kanat-Alexander 2010-03-01 12:54:03 PST
Although I think CONTENT_LENGTH might not be the right item, since it's the length of the headers that matters. Perhaps simply checking the URL length would work. You could put a new parameter in the Advanced section, for trunk, and just make it a constant on the branch.
Comment 10 David Lawrence [:dkl] 2010-03-01 15:08:37 PST
Created attachment 429612 [details] [diff] [review]
Patch to not redirect in buglist.cgi if URI is too long for 3.4 (v1)
Comment 11 David Lawrence [:dkl] 2010-03-01 15:09:09 PST
Created attachment 429613 [details] [diff] [review]
Patch to not redirect in buglist.cgi if URI is too long for 3.6 (v1)
Comment 12 Max Kanat-Alexander 2010-03-01 15:12:52 PST
Comment on attachment 429612 [details] [diff] [review]
Patch to not redirect in buglist.cgi if URI is too long for 3.4 (v1) 

>=== modified file 'Bugzilla/Constants.pm'
>+    my $uri_length = length(correct_urlbase() . "buglist.cgi" . $cgi->canonicalise_query());

  Here you can just check the length of $cgi->self_url, I'm pretty sure.

>+    if (!CGI_URI_LIMIT || $uri_length < CGI_URI_LIMIT) {
>+        print $cgi->redirect(-url => $cgi->url(-relative => 1, -query => 1));

  I don't think that the redirect() code needs to be changed, does it?
Comment 13 Max Kanat-Alexander 2010-03-01 15:13:05 PST
Comment on attachment 429613 [details] [diff] [review]
Patch to not redirect in buglist.cgi if URI is too long for 3.6 (v1) 

  Thinking about it and looking at both these patches, I just want to go with the 3.4 implementation--I'm realizing that a parameter isn't really needful.
Comment 14 David Lawrence [:dkl] 2010-03-01 15:17:04 PST
(In reply to comment #12)
> (From update of attachment 429612 [details] [diff] [review])
> >=== modified file 'Bugzilla/Constants.pm'
> >+    my $uri_length = length(correct_urlbase() . "buglist.cgi" . $cgi->canonicalise_query());
> 
>   Here you can just check the length of $cgi->self_url, I'm pretty sure.

I wasn't sure without looking at the CGI.pm code if self_url had the raw query string or the current adjusted one. I know query_string() is the raw string so I fel best to use canonicalise_query() to be sure.

> >+    if (!CGI_URI_LIMIT || $uri_length < CGI_URI_LIMIT) {
> >+        print $cgi->redirect(-url => $cgi->url(-relative => 1, -query => 1));
> 
>   I don't think that the redirect() code needs to be changed, does it?

It is just indented. I didn't change the actual code there.

Dave
Comment 15 Max Kanat-Alexander 2010-03-01 16:11:06 PST
I'll still take this for 3.4.6 if it comes in before we do the release.
Comment 16 David Lawrence [:dkl] 2010-03-02 08:06:33 PST
(In reply to comment #14)
> > >+    if (!CGI_URI_LIMIT || $uri_length < CGI_URI_LIMIT) {
> > >+        print $cgi->redirect(-url => $cgi->url(-relative => 1, -query => 1));
> > 
> >   I don't think that the redirect() code needs to be changed, does it?

Ah, my bad. I see now what you meant. That was me copying the same code from the 3.6 patch which I realize the redirect() is different.

New patch coming.
Comment 17 David Lawrence [:dkl] 2010-03-02 08:46:35 PST
Created attachment 429752 [details] [diff] [review]
Patch to not redirect in buglist.cgi if URI is too long for 3.4/3.6 (v2) 

Ok. Switched to using $cgi->self_url and also use same patch now for both 3.4 and 3.6 which works fine for me.
Comment 18 Max Kanat-Alexander 2010-03-02 15:03:20 PST
Comment on attachment 429752 [details] [diff] [review]
Patch to not redirect in buglist.cgi if URI is too long for 3.4/3.6 (v2) 

>=== modified file 'Bugzilla/Constants.pm'
>+    if (!CGI_URI_LIMIT || $uri_length < CGI_URI_LIMIT) {

  I wouldn't even check ! on the constant.

  Otherwise, this looks fine; the above can be fixed on checkin.
Comment 19 David Lawrence [:dkl] 2010-03-03 14:13:26 PST
3.4:
modified buglist.cgi
modified Bugzilla/Constants.pm
Committed revision 6735.

3.6:
modified buglist.cgi
modified Bugzilla/Constants.pm
Committed revision 7010.

trunk:
modified buglist.cgi
modified Bugzilla/Constants.pm
Committed revision 7041.
Comment 20 David Lawrence [:dkl] 2010-03-04 08:27:14 PST
Actually after applying this to our code and getting ready to push to production. I feel we should lower the limit from 10000 to 8000 since the Apache default URI limit is 8190 and most people will probably not change that. So if we have it set to 10000, then it is still possible to get the error. Thoughts?

http://httpd.apache.org/docs/2.2/mod/core.html#limitrequestline

Dave
Comment 21 Max Kanat-Alexander 2010-03-04 16:30:16 PST
  Yeah, reducing it to 8000 is a good idea. You can fix that with a second checkin; make sure to still set --fixes on the commit, though.
Comment 22 David Lawrence [:dkl] 2010-03-04 20:43:52 PST
(In reply to comment #21)
>   Yeah, reducing it to 8000 is a good idea. You can fix that with a second
> checkin; make sure to still set --fixes on the commit, though.

And it keeps getting better :) Apparently it was made known to me that although buglist.cgi now works for querys over 8k, the other links on the page, such as Change Columns, CSV, Feed, etc. and probably the header links for sorting still give the Request-URI Too Large error. Should I reopen this bug and try to find a solution or open a separate bug?

Thanks
Dave
Comment 23 Max Kanat-Alexander 2010-03-04 21:29:17 PST
  I believe there is already a separate bug for the general idea that URLs can be too long, but if there isn't, you can file a separate bug. In any case, keep this one FIXED.
Comment 24 David Lawrence [:dkl] 2010-03-05 10:52:32 PST
(In reply to comment #21)
>   Yeah, reducing it to 8000 is a good idea. You can fix that with a second
> checkin; make sure to still set --fixes on the commit, though.

Committed to 3.4, 3.6, and trunk.
modified Bugzilla/Constants.pm

Dave

Note You need to log in before you can comment on or make changes to this bug.