Bug 54946 - Missing Reason-Phrase in HTTP response header for *standard* 3-digit status codes
Summary: Missing Reason-Phrase in HTTP response header for *standard* 3-digit status c...
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: Core (show other bugs)
Version: 2.4.4
Hardware: Other Linux
: P2 critical (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-09 19:13 UTC by James Dwyer
Modified: 2014-02-17 13:44 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description James Dwyer 2013-05-09 19:13:47 UTC
We moved from Apache 2.2.22 to 2.2.24 and found a change in behavior that is breaking our embedded non-changeable clients. This behavior is changed in 2.2.4 and on the HEAD also.

Previously, when we used Apache 2.2.22 and Phusion Passenger 3.0.14 with a Rails/Sinatra app that returned any 3-digit status code like 200, we would get a http status-line pulled from a standard table like:

HTTP/1.1<sp>200<sp>OK

Now, in Apache 2.2.24 we are getting:

HTTP/1.1<sp>200<sp>

We tracked this down into a change made for apache bug 44995 that caused this new behavior. The bug was a fix to handle custom status codes, but it also ended up breaking standard codes such that it no longer returned standard reason-phrase enhanced responses.

I realize that the RFC2616 specification allows for the HTTP/1.1<sp>200<sp> response, but our client is not changeable and crashes on receiving no reason-phrase.

The code in question is in http_filters.c method validate_status_line(r).

Previously in 2.2.22 it did:

if (  len <= 4 ) ... r->status_line = NULL

and then in http_filters.c method basic_http_header_check(r,protocol) it would:

if ( !r->status_line) {
  r->status_line = ap_get_status_line(r->status);
}

which would effectively convert the 3-digit status code into a full status line complete with matching reason-phrase. I think the intention for the code is that if you have a 3-digit status code that it would look up in ap_get_status_line to find a standard reason-phrase

The new code in 2.2.24 in http_filters.c method validate_status_line(r) does:

if ( len < 3 ... ) {
... 
} else if ( len == 3 ) {
  r->status_line = apr_pstrcat(r->pool, r->status_line, " ", NULL);
}

which appends the space at the end. The http_filters.c call to ap_get_status_line therefore never gets triggered and the standard 3-digit code does not get the full matching reason-phrase. Notice that previously if the status code was 4 or less in length, and valid, it would NULL it out, but now it adds the space instead for *all* codes standard or custom.

I don't think the intention of the bug fix in 44995 was to affect the behavior for standard status code, but only change the behavior for custom status codes.

We made a simple change to the new code to check for len <= 4 and it started working again, so this is definitely the cause of the new behavior. But this of course, reverted the enhancement in 44995

A fix for this, then, would be to change the logic to something like this:

- If it's a 3-digit status code (NNN)
   - If it's known (i.e. has a valid response from ap_get_status_line) then use the valid response.
   - If it's unknown (i.e. has an invalid response from ap_get_status_line) then set status line to (NNN<sp>)
- If it's a 4-digit status code (NNN<sp>) already, then filter did not intend for there to be a reason phrase, so use that as the status_line
- The other logic would remain the same for nulling it out if it is improperly formed and returning 500 if the code can't figure out what to do with it.

This would cause non-standard responses to return NNN<sp> and standard status codes to be converted to their standard reason phrase.

To test this you can use jsp:
  <%response.setStatus(200);%> 
or a rails controller action:
  def standard_response
    render :nothing => true, :status => 200
  end
or a sinatra action:
  get '/standard_response' do
    200
  end

Please let me know if you need any help seeing this problem, or understanding it. It is not allowing us to move to the latest secure apache without breaking our clients.
Comment 1 James Dwyer 2013-05-09 19:34:15 UTC
I maybe should mention that the client is pretty common, there are over 50 million of them in the wild and used daily (and not just by us). Just to give some idea of the scope of the issue that it might be encountered more and more as people upgrade their servers.
Comment 2 Rainer Jung 2013-05-09 20:29:59 UTC
Note that the change was *not* introduced in 2.2.24. You are comparing 2.2.22 with a 2.4 release, not with 2.2.24. The cited code does not differ between 2.2.22 and 2.2.24 and the change you noted is in 2.4.

Please test with latest 2.4.4 and report back if the problem is still there.
Comment 3 James Dwyer 2013-05-09 21:00:47 UTC
Oh, got it. I was given to believe that I was comparing vanilla apache 2.2.22 and 2.2.24 but we had a version that had been already patched by someone else. I will perform this test with vanilla 2.4.4 and change the status on this if it is broken on that version. 

And I confirmed what you had said that 2.2.24 does not have that code change, thanks for the clarification.
Comment 4 Rainer Jung 2013-05-09 21:53:29 UTC
If the problem persists with 2.4.4, can you please also check the following patch:

http://people.apache.org/~rjung/patches/status_code-2_4.patch

If haven't testet it myself, but it would be useful if you could confirm it helps.

Thanks.
Comment 5 James Dwyer 2013-05-14 04:54:56 UTC
I confirmed that the undesired behavior (returns 200<sp>) is in 2.4.4.

I also tried your patch and it successfully returns 200 OK instead of 200<sp>. I haven't done any other tests or verification other than that basic case of a 200 status code.

Thanks for the quick response!
Comment 6 Eric Covener 2013-05-14 13:18:37 UTC
(In reply to comment #3)
> Oh, got it. I was given to believe that I was comparing vanilla apache
> 2.2.22 and 2.2.24 but we had a version that had been already patched by
> someone else. I will perform this test with vanilla 2.4.4 and change the
> status on this if it is broken on that version. 

If you can share -- did you hit this on 2.2.24 on EC2?
Comment 7 Justin Honold 2013-05-14 15:09:24 UTC
So I had run into this yesterday.  For me it's disrupting the use of Varnish.  I have a Rails + Passenger + httpd stack behind Varnish, and Varnish uses "probes" to check for site availability.  The method we currently have in place is to render a blank page, which is sufficient to generate a 200 response.  As it turns out, Varnish appears to care about the Reason-Phrase even if it receives a 200 - and now it doesn't have one, so Varnish thinks the apps are down.  As a workaround I'm forcing them to be "healthy" in the admin console, but doing this prevents Varnish from detecting failure on a given app server.

I mentioned to Eric I was on 2.2.24 on Amazon Linux/EC2, which is why he asked.  I just downloaded their SRPM, and the patch at https://issues.apache.org/bugzilla/attachment.cgi?id=22019&action=diff is present via "httpd-2.2.3-r693108.patch," which references:

https://bugzilla.redhat.com/show_bug.cgi?id=853128 (no access)
http://svn.apache.org/viewvc?view=revision&revision=693108
http://svn.apache.org/viewvc?view=revision&revision=693224

So there's the smoking gun - at least for me.
Comment 8 James Dwyer 2013-05-14 15:20:37 UTC
Yeah, that's where we saw it, and why I initially thought it was in the 2.2 branch. We're going through them now to try and revert that patch or add this fix on their end, and then hopefully this bugzilla issue will get it fixed on 2.4.

Returning this to the new status as I have added the requested information.
Comment 9 Justin Honold 2013-05-14 17:03:55 UTC
Note: it's also present on httpd-2.2.15-28.el6.centos.x86_64 (CentOS 6), which would also put it in RHEL 6.
Comment 10 Justin Honold 2013-05-16 17:02:09 UTC
Is submitting Rainer's patch the next step to resolution?
Comment 11 Rainer Jung 2013-05-16 21:00:26 UTC
If noone beats me to it I will do that next week.

Rainer
Comment 12 James Dwyer 2013-07-01 22:57:51 UTC
Just wondering if this got committed or if it is still outstanding? Thanks!
Comment 13 Rainer Jung 2013-10-03 21:55:19 UTC
Applied to trunk in r1529014 and proposed for backport to 2.4.
Comment 14 Rainer Jung 2013-11-19 21:29:28 UTC
Applied to 2.4.x in r1530280.Will be part of 2.4.7.
Comment 15 Justin Honold 2013-11-20 16:34:51 UTC
Any backports for 2.2?  It's the stock HTTPD on the most-current release of Red Hat Enterprise Linux (and thus CentOS), for example.

They're also still on 2.2.15, but I would think it would lend the patch a lot of credibility.
Comment 16 Eric Covener 2013-11-22 19:08:51 UTC
(In reply to Justin Honold from comment #15)
> Any backports for 2.2?  It's the stock HTTPD on the most-current release of
> Red Hat Enterprise Linux (and thus CentOS), for example.
> 
> They're also still on 2.2.15, but I would think it would lend the patch a
> lot of credibility.

Justin, I think your affected 2.2 has a 2.4 backport or some other kind of patch.  2.2 has always thrown away 1-4 character status lines, which get replaced by the one the server would generate.
Comment 17 Eric Covener 2014-01-20 00:12:11 UTC
Nothing more the ASF can do for the distribution Justin is on.