Bug 57808

The heap dumps would be around 7MB compressed so I can't attach them. Where should I post them?

In most cases, 5MB would be a trivial amount of memory, so how about using a flag with a system property for that (or something similar) ?

This preloading is necessary to avoid performance bottlenecks. (See history of that code, or search Bugzilla  -> bug 51400).

As such, it cannot be removed.

If one wants, it shall be possible to provide a user-configurable list of charsets as an opt-in feature (the same as suggested in comment 2), but Tomcat will behave as all the other charsets do not exist.

I have read bug 51400 and my understanding is that preloading is not necessary to avoid performance bottlenecks. Preloading was discussed but I couldn't find the reasons why preloading was ultimately chosen. The reasons for other implementation decisions I could find (not caching misses, not storing permutations, …).
My understanding is that avoiding calling (indirectly) Charset.forName() / Charset.lookup() during normal request processing is necessary to avoid performance bottlenecks. This is achieved by the caching even when avoiding the preloading.

Created attachment 32662 [details]
2nd try at a patch

Updated patch:
- keeps the current behavior as default
- new behavior is available and configurable through a system property
- document the system property

(In reply to Philippe Marschall from comment #4)
> I have read bug 51400 and my understanding is that preloading is not
> necessary to avoid performance bottlenecks. Preloading was discussed but I
> couldn't find the reasons why preloading was ultimately chosen.

It means you don't have to cache the misses since misses have their own DoS potential.

I wonder if lazy-loading the cache with ISO-8859-1 and UTF-8 pre-loaded would give a sensible balance without adding another configuration option?

I'd feel more comfortable in these discussions with some numbers to quantify the performance impact.

(In reply to Mark Thomas from comment #6)

> It means you don't have to cache the misses since misses have their own DoS
> potential.

Is it correct that there are two possible DOS attacks with dynamic charset loading, i.e with the patch applied:

 1. force permanent increased memory during runtime with requests that specify
    all avaliable but previously unloaded charsets.

 2. force expensive charset name/instance lookup misses with requests
    that specify non-existent charset names.

Any other?

The consequence of threat 1 is limited. It is self-exhausting as soon as all available charsets have been loaded once. One mitigation could be to use weak references, if it's worth it?

Threat 2 could be mitigated if charset loading is performed only when the charset name is valid. Preloading all available charset names on startup will block threat 2, right? Assuming of course that prefetching all charset names doesn't nullify the memory reduction.

BTW, assuming the proposal is accepted, I'd like to see dynamic loading as the new default, and if necessary an option to make tomcat fall back to the old pre-loading behaviour. Correct and lean should be the default, not an option.

(N.B I'm not a tomcat committer, just a interested user.)

(In reply to Fredrik Jonson from comment #7)
> (In reply to Mark Thomas from comment #6)
> 
> > It means you don't have to cache the misses since misses have their own DoS
> > potential.
> 
> Is it correct that there are two possible DOS attacks with dynamic charset
> loading, i.e with the patch applied:
> 
>  1. force permanent increased memory during runtime with requests that
> specify
>     all avaliable but previously unloaded charsets.
> 
>  2. force expensive charset name/instance lookup misses with requests
>     that specify non-existent charset names.

Just to nit pick, the expensive lookups are (at least were when this code was written) for any Charset (valid or invalid) that are not in the cache.

> Any other?
> 
> The consequence of threat 1 is limited. It is self-exhausting as soon as all
> available charsets have been loaded once.

Agreed.

> One mitigation could be to use weak references, if it's worth it?

I don't think so.

> Threat 2 could be mitigated if charset loading is performed only when the
> charset name is valid. Preloading all available charset names on startup
> will block threat 2, right?

Correct.

> Assuming of course that prefetching all charset
> names doesn't nullify the memory reduction.

I think it does. At keast the way we get the names of the valid charsets currently it does,

> BTW, assuming the proposal is accepted, I'd like to see dynamic loading as
> the new default, and if necessary an option to make tomcat fall back to the
> old pre-loading behaviour. Correct and lean should be the default, not an
> option.

No objections there.

> (N.B I'm not a tomcat committer, just a interested user.)

A few good patches can soon change that ;)

The discussion in bug 51400 suggests that the performance characteristics we were trying to avoid have changed in Java 7 (i.e. Tomcat 8 onwards). That makes it more important we have some good test results on which to base any decision.

I've been doing some further investigations and I suspect that the results are highly dependent on the JVM version used.

With recent Java 8 releases the memory retained by this cache is relatively low (around 100k) and while you can reduce that by not preloading, the gains you make are lost by what you have to add to account for lazy-loading while retaining the performance benefits.

Given that there is not a compelling case for changing the current behaviour, I am resolving this as WONTFIX.

FYI:
A new enhancement request - bug 58859 "Allow to limit charsets / encodings supported by Tomcat" - offers a similar feature, but with different requirements and background.