Bug 64619 - Regression: Removal of scratchdir fallback affects existing code
Summary: Regression: Removal of scratchdir fallback affects existing code
Status: RESOLVED WONTFIX
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: Jasper (show other bugs)
Version: 8.5.57
Hardware: PC All
: P2 minor (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-22 08:57 UTC by Markus Schlegel
Modified: 2020-08-12 15:20 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Schlegel 2020-07-22 08:57:46 UTC
https://github.com/apache/tomcat/commit/6d7e1a00cd706654eca445989d85960cbfba83ac assumes, that the scratchdir is always set. But this is not necessarily the case, especially for tomcat embedded.

Adding this code change in a new mayor release along with an entry in the changelog/releasenotes wouldn't be a problem.
But adding this change in a minor release, without notice in the changelog and without a real need (it does not fix a bug or security issue) IS a problem. 

Users of Tomcat-embedded should be able to rely on the changelog, such that applied security fixes can be integrated as fast as possible into the dependent applications.
Therefore, please don't do unforced refactorings in a minor maintenance release.

ServletContext is an interface which can have many sorts of implementations. We use our own implementation of that interface for various reasons. You can not rely on the assumption, that the Attribute ServletContext.TEMPDIR is always set.
Comment 1 mgrigorov 2020-07-22 09:17:26 UTC
This change has been made first in Tomcat 10.x (master) branch and then downported to 8.5.x to keep the codebases in sync.


Tomcat sets the attribute to the ServletContext at
org.apache.catalina.core.StandardContext#postWorkDirectory(). 

StandardContext is a facade for the implementation of ServletContext. So it does not matter whether you use a custom impl of ServletContext or not. The question is: Why org.apache.catalina.core.StandardContext#postWorkDirectory() is not called in your case ?
postWorkDirectory() is called by org.apache.catalina.core.StandardContext#startInternal()
Comment 2 Mark Thomas 2020-07-22 09:33:05 UTC
The Tomcat code base being the age and size it is, it isn't uncommon to come across cruft while researching other issues. The decision when and how to remove such cruft is always going to be taken on a case by case basis.

In this case it is a mandatory requirement of the Servlet specification (see Servlet 3.1, section 4.8.1) that a ServletContext implementation provides a temporary storage location and exposes it via the javax.servlet.context.tempdir attribute. While the Servlet 3.1 spec applies to Tomcat 8.x, the same requirement has existed at least as far back as Servlet 2.2 (December 1999 / Tomcat 3).

Given the above, removal of the code intended to address a situation that should not have existed for over 20 years and looked to be addressing pre Tomact 3 implementations seemed reasonable.

Whether or not the clean-up should have been included in the changelog is debatable. With hindsight, inclusion may have helped in this case. There is a balance to strike between including everything in the changelog and not overwhelming the changelog with trivial entries. I think there is probably a case for moving that balance a little more towards including entries.

At this point this looks more like a specification compliance bug in the custom ServletContext implementation rather than a Tomcat issue.
Comment 3 Markus Schlegel 2020-07-22 13:09:36 UTC
Thanks for the clarification.
I can understand your demand for having the codebase of the maintenance branches somewhat in sync.
Since the checkin was not associated with a issue, I thought that it was an unforced change.

After looking into the spec given by markt, I understand that it indeed was a bug to silently share the same working-directory across different servlet context when javax.servlet.context.tempdir is not defined.
Having a "must-have" attribute defined in the servlet spec seems very unfortune for me. The spec would better have made an additional method on the interface for this, such that the change and need becomes obvious.

But finally, I will have to fix that in our code.
Nevertheless, it would have been a good practice to create an issue for this code change and explain the reasoning for the change there (with reference to specification). My application would still have failed to run, but it would have been clear why.

@mgrigorov: We have different usages of Tomcat in our Application. The "normal" WebServer/Servlet-Container usage is not affected by this issue. Another usage uses only Jasper as a component of Tomcat to generate reports using JSP's without being inside a real web-container. Only minor involvement of catalina there and therefore no call to postWorkDirectory().