#321 check for return after response.sendRedirect()

[Piero Ottuzzi]

Hi,

it is very easy to forget that response.sendRedirect() does not return and so it easy to let the program flow after a response.sendRedirect().
Is it possible to add this check?

Hope I was clear... please ask for more informations as needed.

Thanks
Bye
Piero

[Andrey Loskutov]

Piero,
could you please attach the small example of the "bad" code you want to check?
I have no idea what type "response" has, why it does not return and why it should be a problem.
Regards,
Andrey

[Tagir Valeev]

Andrey, he probably says about HttpServletResponse#sendRedirect. I disagree that code must immediately return after this. It's possible that some logging or clean up still has to be done after sending the redirect. We probably can check if somebody works with the same response after redirecting and there were no returns, throws nor jump-outs between the redirect and further work with response. This way we will warn in such cases:

if(somethingWrong) {
  response.sendRedirect(blahblah);
  // return is missing here
}
OutputStream out = response.getOutputStream();
... // send some other response

Probably the same can be done for response.sendError(). Still research is necessary to properly classify which methods should not be called after sendRedirect/sendError and how to check properly that there's a possible control flow from sendRedirect/sendError to that next method call. Do we already have any detectors which report incorrect sequence of method calls? Probably we can create a generalized detector (if we don't have it yet) which can issue various similar warnings about wrong sequence of calls (for example, trying to write into OutputStream after closing it).

In any case Andrey is right. Piero, please attach an example of code where you want the warning to be issued.

[Piero Ottuzzi]

Hi,

thank you for your answer.
The context is J2EE and specifically web applications; I'm speaking about HttpServletResponse[1] and the method sendRedirect() that returns void.
Usually when you use sendRedirect() you expect to redirect user to another web page and this usually happens: usually programmers follow this logical movement (I'm redirecting user to another web page and code stops here to execute) but this is not reality. If after response.sendRedirect() you do not return the code will continue to execute even if it is not important anymore.
So.... to give you and example:

BAD CODE

if (error)
{
    response.sendRedirect("/error.jsp");
}

doSomethingMore();
doSomethingMoreAgain();
saveDataSomeWhere();

GOOD CODE

if (error)
{
    response.sendRedirect("/error.jsp");
    return;
}

doSomethingMore();
doSomethingMoreAgain();
saveDataSomeWhere();

As stated before, in bad code, after the response.sendRedirect() line the following methods will be executed!

Hope this is clear :)

Thanks
Bye
Piero

[1]https://docs.oracle.com/javaee/6/api/javax/servlet/http/HttpServletResponse.html#sendRedirect(java.lang.String)

[William Pugh]

Piero,
Based on your description, while the code shouldn't write anything else to the http response after calling sendRedirect, it is perfectly OK for the servlet to do cleanup code after the call to sendRedirect. This makes the analysis a little more complicated, as we have to distinguish between good and bad code to have after a call to sendRedirect.

Bill

[Piero Ottuzzi]

Hi,

I totally agree with all your observations: as stated by Tagir we should raise a warning if after a response.sendRedirect() or a response.sendError() someone still uses request (HttpServletRequest) or response objects again.

[Tagir Valeev]

Using request object seems to be ok. For example in the following code:

void redirect(HttpServletRequest request, 
              HttpServletResponse response, String target)
                  throws IOException {
    response.sendRedirect(target);
    log.info("Redirect to "+target+" is successfully sent. "+
      "The request URI was "+request.getRequestURI());
}

Note that we cannot swap redirect and logging lines as this will break the logic in case of IOException.

[Piero Ottuzzi]

Hi again,

thank you all very much for your thoughtful answers: you are right and I never thought so deeply about this problem.
After all we can catch a new access to response object: in the javadoc for HttpServletResponse.sendError() and HttpServletResponse.sendRedirect() we can read 'After using this method, the response should be considered to be committed and should not be written to.'.
So after all I think it would be right to alert the user if after a sendRedirect()/sendError() we use response object again.

Thanks
Bye
Piero

[Tagir Valeev]

I've tried to implement it as OpcodeStackDetector detecting HttpServletResponse case as well as using Reader/Writer/Stream after calling close(). Unfortunately the results were quite poor. I did not find any real bugs in tested code, only several false-positives due to non-trivial control flow on exception handling. Thus I will not commit this implementation. I think, such detector should be written using CFG dataflow, but currently I'm not experienced enough to implement it. Probably Bill or Andrey can do this. Anyways it looks like that even with properly written detector we will rarely catch real bugs.

Piero, it would be really nice if you submit a piece of real code (probably simplified, but real) where such bug was actually discovered.