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 ?).
Rémy
> Mark
>
>
> > +
> > + inputBuffer = new Http11InputBuffer(request,
> > protocol.getMaxHttpRequestHeaderSize(), httpParser);
> > request.setInputBuffer(inputBuffer);
> >
> > outputBuffer = new Http11OutputBuffer(response,
> > protocol.getMaxHttpResponseHeaderSize());
> > @@ -752,7 +764,7 @@ public class Http11Processor extends AbstractProcessor {
> > // Validate the characters in the URI. %nn decoding will be
> > checked at
> > // the point of decoding.
> > for (int i = uriBC.getStart(); i < uriBC.getEnd(); i++) {
> > - if (!protocol.getHttpParser().isAbsolutePathRelaxed(uriB[i])) {
> > + if (!httpParser.isAbsolutePathRelaxed(uriB[i])) {
> > badRequest("http11processor.request.invalidUri");
> > break;
> > }
> > diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
> > index 4a38201139..2353c25812 100644
> > --- a/webapps/docs/changelog.xml
> > +++ b/webapps/docs/changelog.xml
> > @@ -225,6 +225,13 @@
> > Align buffer reuse of the OpenSSLEngine for tomcat-native with
> > the FFM
> > code. (remm)
> > </update>
> > + <fix>
> > + Create the <code>HttpParser</code> in <code>Http11Processor</code>
> > + if it is not present on the <code>AbstractHttp11Protocol</code>
> > + to provide better lifecycle robustness for regular HTTP/1.1. The
> > new
> > + behavior was introduced on a previous refactoring to improve HTTP/2
> > + performance. (remm)
> > + </fix>
> > </changelog>
> > </subsection>
> > <subsection name="Jasper">
> >
> >
> > ---------------------------------------------------------------------
> > 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
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
