On 11/03/2019 21:19, Michael Osipov wrote: > Am 2019-03-11 um 09:03 schrieb Rainer Jung:
<snip/> >> I think Mark refers to this one: >> >> https://marc.info/?l=tomcat-dev&m=153856675022101&w=2 > > Thanks Rainer. > > So this is a fix for another issue which clearly causes a regression. Sort of. A failure was reported when running some of the unit tests on Windows. That was fixed in this commit: https://github.com/apache/tomcat/commit/62c04ea42f115533437bccaa7873416abecc8c6e#diff-717001e0451788fe9f26d1176a4fff54 That fix had the potential to hide future case-sensitivity regressions so it was re-worked in: https://github.com/apache/tomcat/commit/db71c925106915581f1b60b0fda9c352fcdd9138#diff-717001e0451788fe9f26d1176a4fff54 While investigating the failures above it was noticed that ContextConfig was inconsistent. If the user supplied an absolute value for the docBase, the canonical version of that path was set as the docBase on the Context. If the user supplied a relative value for the docBase, the value was passed as is. There is no good reason that I can see for that inconsistency and potential (unproven) for things to break if the docBase contained sequences like "../..". Hence: https://github.com/apache/tomcat/commit/fd2abbb525660a9968694afd99a58f8c22cb54c6 There are actually 3 different options for making those calls consistent: - getPath() - getAbsolutePath() - getCanonicalPath() getAbsolutePath() wasn't considered as it wasn't one of the options already in use and the preference was to keep change to a minimum. The value (part of it anyway) ends up stored in the Context docBase and the general expectation in the code is that that value is absolute / canonical hence getCanonicalPath() was chosen. > Looking into RFC 8089 (proposed), E.2 talks about canonicalization of > driver letters. The behaviour described there is consistent with observed behaviour with current Microsoft operating systems. Based on experience, I'd rather ask the OS what that it considers to the canonical path than rely on an RFC. > This needs to be taken care of solving both issues. > > @Guido, please create a BZ issue with a minimal test case. It is, and it isn't, quite that simple. I've spent a chunk of time today experimenting with this code to see what the impacts are, if any, of reverting to using getPath() here. On the plus side, I haven't been able to find a combination of code and configuration that breaks with either getPath() or getCanonicalPath(). I haven't tested it but I'm confident that if both those methods are OK then getAbsolutePath() will be OK as well. What I have also noticed is what seems like a large number of calls to getAbsolutePath() and getCanonicalPath(). Those are relatively expensive calls and I am wondering if there is scope for some performance improvement here. That needs a little more research as I was doing a lot of restarts and not paying attention to whether those calls were during restart or during request processing. Reducing calls during request processing will have a bigger performance improvement. Where things get interesting is, if there is scope to reduce the number of calls to getAbsolutePath() and/or getCanonicalPath(), to what extent do those improvements depend on the current code remaining as is? Looking at the code in ContextConfig.fixDocBase() it looks like it should be possible to switch lines 585 and 587 to use getAbsolutePath() without having too much impact on any performance improvements we may want to consider. That should address the regression. @Guido can you confirm that please? I can run the unit tests and if they pass and the correction of the regression is confirmed it should be possible to get the fix into the next set of releases. The next release is almost certainly too soon for completing the performance review. That is probably going to need to wait until the following set of releases. Mark --------------------------------------------------------------------- To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org For additional commands, e-mail: users-h...@tomcat.apache.org
