Bug 46051 - Servlet response writer does not respect line.separator system property
Servlet response writer does not respect line.separator system property
Status: RESOLVED FIXED
Product: Tomcat 6
Classification: Unclassified
Component: Catalina
unspecified
All All
: P2 normal (vote)
: default
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2008-10-21 10:54 UTC by Chris Hubick
Modified: 2008-11-01 19:31 UTC (History)
0 users



Attachments
remove special CoyoteWriter println handling (734 bytes, patch)
2008-10-21 10:54 UTC, Chris Hubick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Hubick 2008-10-21 10:54:42 UTC
Created attachment 22763 [details]
remove special CoyoteWriter println handling

public void doGet(final HttpServletRequest request, final HttpServletResponse response) throws ServletException, IOException {
  response.setContentType("text/plain");
  PrintWriter writer = response.getWriter();
  writer.print("hello world");
  writer.println(); //FIXME ignores System.getProperty("line.separator") and always outputs \r\n
  return;
}

This means that if you use a servlet Writer on a Unix system to output any text based data format which is defined as requiring Unix format line endings ('\n'), that data will be corrupted.

This appears to be because org.apache.catalina.connector.CoyoteWriter does:
--------
private static final char[] LINE_SEP = { '\r', '\n' };

public void println() {
  write(LINE_SEP);
}
--------

This used to work in Tomcat 4.

Looking at the old code at http://svn.apache.org/repos/asf/tomcat/archive/tc4.0.x/tags/tc4.0.6/connectors/coyote/src/java/org/apache/coyote/tomcat4/CoyoteWriter.java it appears to defer line endings to the PrintWriter base class which does the right thing.

Was this change was made to output HTTP headers properly or something?  It specifically seems to override the default behavior to do this.  I don't know the code well enough to determine if the attached patch to revert to the default behavior again would break something?

Thanks for your consideration.
Comment 1 Mark Thomas 2008-10-21 12:23:58 UTC
The change is http://svn.apache.org/viewvc?view=rev&revision=298149

I am tempted to leave this as is. Whilst strict adherence to the PrintWriter interface requires that System.getProperty("line.separator") is used, any truly portable web application is going to have to write it's own new lines anyway to be resilient against the case where it runs on a different platform.

If this was fixed, I would change the initialisation of LINE_SEP.
Comment 2 Chris Hubick 2008-10-21 12:50:48 UTC
I see where you are coming from, and somewhat agree - though I think there is value in strict adherance to the API.  The decision is yours - I just wanted to make sure you were aware of it.

Luckily, I had the code to this servlet and patched it to work around this.

Just to note, we were actually bit by this bug when we upgraded from Tomcat 4 on our CAS ( http://www.ja-sig.org/products/cas/ ) central authentication server.  The Yale CAS server code writes out XML using the response Writer, and after the upgrade this output then contained the additional '\r'.  Of course, this technically shouldn't matter to the XML either, except that the old (2.0.10) mod_cas C client we are using on some old servers apparently doesn't use a real XML parser and breaks reading these line endings in.  We can't upgrade the client in our legacy environment because their newer mod_auth_cas client requires a newer Apache.  So, yeah, I patched the server to work as before, and we are working to upgrade all our old client servers, but as with any enterprise, that takes time.
Comment 3 Mark Thomas 2008-10-22 12:59:36 UTC
Just testing a new way to provide an svn link:
r298149
or
revision 298149

Comment 4 Mark Thomas 2008-10-23 12:43:13 UTC
Testing svn links in mail messages - please ignore.

r298149
or
revision 298149
Comment 5 Mark Thomas 2008-10-23 12:53:23 UTC
Testing svn links in mail messages again - please ignore.

r298149
or
revision 298149
Comment 6 Mark Thomas 2008-11-01 17:49:30 UTC
I've fixed this in trunk. I am not going to propose it for back-porting since it might break something and it doesn't make much sense for app servers anyway. But since there is a spec it will be in 7.x
Comment 7 Chris Hubick 2008-11-01 19:31:46 UTC
Sounds good, thanks! :)