On Fri, Oct 25, 2024 at 4:26 PM Konstantin Kolinko <knst.koli...@gmail.com> wrote: > > пт, 25 окт. 2024 г. в 15:45, Mark Thomas <ma...@apache.org>: > > > > On 24/10/2024 18:47, Rémy Maucherat wrote: > > > On Thu, Oct 24, 2024 at 7:11 PM Mark Thomas <ma...@apache.org> wrote: > > >> > > >> On 24/10/2024 15:56, r...@apache.org wrote: > > >>> This is an automated email from the ASF dual-hosted git repository. > > >>> > > >>> remm pushed a commit to branch main > > >>> in repository https://gitbox.apache.org/repos/asf/tomcat.git > > >>> > > >>> > > >>> The following commit(s) were added to refs/heads/main by this push: > > >>> new 7767b84d42 Create a HttpParser if none is found > > >>> 7767b84d42 is described below > > >>> > > >>> commit 7767b84d422cda47b55125eac892f08ad78883ac > > >>> Author: remm <r...@apache.org> > > >>> AuthorDate: Thu Oct 24 16:55:57 2024 +0200 > > >>> > > >>> Create a HttpParser if none is found > > >>> > > >>> Lifecycle is not enforced in these components, and init actually > > >>> does > > >>> little (JMX, protocol handlers, upgrade protocols; all likely not > > >>> used > > >>> and maybe not desired). > > >>> Normally HTTP/2 is always fine since it has to use init to work. > > >>> Reported NPE in Http11InputBuffer from a user with a "working" > > >>> embedded > > >>> Tomcat before 9.0.93. > > >>> --- > > >>> java/org/apache/coyote/http11/Http11Processor.java | 16 > > >>> ++++++++++++++-- > > >>> webapps/docs/changelog.xml | 7 +++++++ > > >>> 2 files changed, 21 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/java/org/apache/coyote/http11/Http11Processor.java > > >>> b/java/org/apache/coyote/http11/Http11Processor.java > > >>> index 7daa79549a..d34e1772c3 100644 > > >>> --- a/java/org/apache/coyote/http11/Http11Processor.java > > >>> +++ b/java/org/apache/coyote/http11/Http11Processor.java > > >>> @@ -146,11 +146,23 @@ public class Http11Processor extends > > >>> AbstractProcessor { > > >>> private SendfileDataBase sendfileData = null; > > >>> > > >>> > > >>> + /** > > >>> + * Http parser. > > >>> + */ > > >>> + private final HttpParser httpParser; > > >>> + > > >>> + > > >>> public Http11Processor(AbstractHttp11Protocol<?> protocol, > > >>> Adapter adapter) { > > >>> super(adapter); > > >>> this.protocol = protocol; > > >>> > > >>> - inputBuffer = new Http11InputBuffer(request, > > >>> protocol.getMaxHttpRequestHeaderSize(), protocol.getHttpParser()); > > >>> + HttpParser httpParser = protocol.getHttpParser(); > > >>> + if (httpParser == null) { > > >>> + httpParser = new > > >>> HttpParser(protocol.getRelaxedPathChars(), > > >>> protocol.getRelaxedQueryChars()); > > >>> + } > > >>> + this.httpParser = httpParser; > > >> > > >> Maybe move this to protocol.getHttpParser() ? > > >> > > >> That way we get one HttpParser per protocol instance. One per > > >> HttpProcessor requires a noticeable amount of memory. > > > > > > I'm not sure what the user was doing with the lifecycle, probably not > > > calling protocol init or something like that. The log looked > > > incoherent. Basically the code reverts the previous commit if > > > protocol.getHttpParser() == null, which seems fair. > > > Moving it to protocol.getHttpParser() is an idea, but it might need to > > > be volatile and coverity could complain about it. This would make > > > things worse since this situation should not happen at all (maybe I > > > can add a log ?). > > > > Fair enough. A warning log that the correct lifecycle isn't being > > followed seems like a good idea to me. > > Looking at the thread > "Tomcat 9.0.96 first start throws java.lang.NullPointerException. > Works in the same conditions with 9.0.91." > on users@, original reporter provided more logs, and there is a > connector initialisation failure that precedes this issue. > > If a connector fails to initialize, it should not be used to serve > requests. I feel that using such a failed connector may lead to a > whole can of issues. Misconfigured (missing) HttpParser is just one of > them. > > (I mean that in theory a misconfigured connector may mean HTTP instead > of HTTPS, using weak ciphers instead of configured strong ones, and > similar issues.) > > > Thus with updated information on the issue, I see no worth in this > change. We would better fix the lifecycle. (I'll comment on the > original thread on users@).
I was initially thinking about a more custom embedding, where the protocol init would not be called. That is possible since there is no lifecycle in Coyote. There should be some robustness there, esp for something like that HttpParser. So LifecycleBase needs a fix, ok. I'm checking if the fix passes the testsuite ;) Rémy > Best regards, > Konstantin Kolinko > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
