Bug 57455 - Bootstrap doesn't handle paths with one quote
Bootstrap doesn't handle paths with one quote
Status: RESOLVED FIXED
Product: Tomcat 8
Classification: Unclassified
Component: Catalina
8.0.x-trunk
PC All
: P2 trivial (vote)
: ----
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2015-01-17 20:28 UTC by Tobias Stoeckmann
Modified: 2015-01-21 10:12 UTC (History)
0 users



Attachments
Fix with tests (1.17 KB, text/plain)
2015-01-17 20:28 UTC, Tobias Stoeckmann
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Stoeckmann 2015-01-17 20:28:59 UTC
Created attachment 32374 [details]
Fix with tests

During bootstrap, repositories for class loaders are scanned from configuration. The syntax allows to quote paths and/or to simply join them with commas.

If the path starts with a quote ("), Bootstrap#getPaths assumes that it must have hit the first regular expression match and implies that it will end with a quote, too.

This is not correct, "a for example would be allowed due to the second regular expression rule. Bootstrap#getPaths would swallow the last character (a) and proceed.

The fix is simple: Before removing the quotes, make sure that the string actually ends with a quote.

Please see attached diff for latest trunk version of Tomcat 9, which also covers the issue with a new test case.
Comment 1 Mark Thomas 2015-01-19 14:45:53 UTC
Thanks for the patch. I can confirm that it fixes the issues described. But...

This has the potential to get messy. Neither " nor , is permitted in a file name in Windows but both are acceptable in unix and friends.

We started off simply using ',' (comma) as a delimiter. This assumed ',' never appeared in the value. Then we had a bug report that paths containing ',' broke things. To fix this we added optional quoting using '"'. This allowed ',' to appear in the value if the value was quoted. This assumed '"' never appeared in a value. Now we have this bug.

I am concerned that the proposed solution is a sticking plaster rather than a complete fix and that in a few months time we'll have someone raising a bug about a different edge case (I can already see several valid file names that will break this).

Unless we switch to using an escape character of some form, we are always going to have these issues. And I'm not at all sure that escaping will be possible since the values being parsed are obtained - in the default case - via unescaped environment variables.

I'm leaning towards 'fixing' this by adding a comment that paths with '"' are not permitted to catalina.propeties.
Comment 2 Tobias Stoeckmann 2015-01-19 21:14:10 UTC
Totally fine with that approach!
Comment 3 Christopher Schultz 2015-01-20 16:37:34 UTC
(In reply to Mark Thomas from comment #1)
> I'm leaning towards 'fixing' this by adding a comment that paths with '"'
> are not permitted to catalina.propeties.

+1 with an actual check for the condition and an error message, rather than odd-looking behavior, and another bug being filed down the road.
Comment 4 Mark Thomas 2015-01-21 10:12:51 UTC
Fixed in trunk and 8.0.x and will be included in 8.0.18 onwards.