Skip to content

Commit 93be060

Browse files
sbordetsbordet
authored
Merge pull request #13767 from jetty/fix/jetty-12.1.x/13615-mixed-headers
Fixes #13615 - Concurrency issue, headers from different requests are mixed in Jetty 12.0.27.
2 parents c17aa18 + ab784c3 commit 93be060

File tree

16 files changed

+842
-162
lines changed

16 files changed

+842
-162
lines changed

‎jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java‎

Lines changed: 51 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -394,56 +394,69 @@ private void onNewStream(Callback callback)
394394
private void onHeaders(HeadersFrame frame, Callback callback)
395395
{
396396
MetaData metaData = frame.getMetaData();
397-
boolean isTrailer = !metaData.isRequest() && !metaData.isResponse();
398-
if (isTrailer)
397+
Throwable metaDataFailure = MetaData.Failed.getFailure(metaData);
398+
if (metaDataFailure != null)
399399
{
400-
// In case of trailers, notify first and then offer EOF to
401-
// avoid race conditions due to concurrent calls to readData().
402-
boolean closed = updateClose(true, CloseState.Event.RECEIVED);
403-
notifyHeaders(frame, Callback.from(() ->
404-
{
405-
// Offer EOF in case the application calls readData() or demand().
406-
if (offer(Data.eof(getId())))
407-
processData(true);
408-
if (closed)
409-
getSession().removeStream(this);
410-
}, callback));
400+
// Request failures are notified by the session,
401+
// since the Stream.Listener won't be available yet.
402+
assert !metaData.isRequest();
403+
404+
// It's a bad response (or trailer), response content will be dropped.
405+
onFailure(new FailureFrame(ErrorCode.PROTOCOL_ERROR.code, null, metaDataFailure), callback);
411406
}
412407
else
413408
{
414-
long length = -1;
415-
HttpFields fields = metaData.getHttpFields();
416-
boolean connect = HttpMethod.CONNECT.is(request.getMethod());
417-
if (fields != null && !connect)
418-
length = fields.getLongField(HttpHeader.CONTENT_LENGTH);
419-
dataLength = length;
420-
421-
// Offer EOF for either the request or the response in
422-
// case the application calls readData() or demand().
423-
boolean eof = frame.isEndStream() && offer(Data.eof(getId()));
424-
425-
// Requests are notified to a Session.Listener, here only notify responses.
426-
if (metaData.isRequest())
409+
boolean isTrailer = !metaData.isRequest() && !metaData.isResponse();
410+
if (isTrailer)
427411
{
428-
callback.succeeded();
429-
}
430-
else
431-
{
432-
MetaData.Response response = (MetaData.Response)metaData;
433-
if (connect && response.getStatus() != HttpStatus.OK_200)
434-
{
435-
// A failed tunnel attempt, must close the request side.
436-
updateClose(true, CloseState.Event.AFTER_SEND);
437-
}
438-
boolean closed = updateClose(frame.isEndStream(), CloseState.Event.RECEIVED);
412+
// In case of trailers, notify first and then offer EOF to
413+
// avoid race conditions due to concurrent calls to readData().
414+
boolean closed = updateClose(true, CloseState.Event.RECEIVED);
439415
notifyHeaders(frame, Callback.from(() ->
440416
{
441-
if (eof)
417+
// Offer EOF in case the application calls readData() or demand().
418+
if (offer(Data.eof(getId())))
442419
processData(true);
443420
if (closed)
444421
getSession().removeStream(this);
445422
}, callback));
446423
}
424+
else
425+
{
426+
long length = -1;
427+
HttpFields fields = metaData.getHttpFields();
428+
boolean connect = HttpMethod.CONNECT.is(request.getMethod());
429+
if (fields != null && !connect)
430+
length = fields.getLongField(HttpHeader.CONTENT_LENGTH);
431+
dataLength = length;
432+
433+
// Offer EOF for either the request or the response in
434+
// case the application calls readData() or demand().
435+
boolean eof = frame.isEndStream() && offer(Data.eof(getId()));
436+
437+
// Requests are notified to a Session.Listener, here only notify responses.
438+
if (metaData.isRequest())
439+
{
440+
callback.succeeded();
441+
}
442+
else
443+
{
444+
MetaData.Response response = (MetaData.Response)metaData;
445+
if (connect && response.getStatus() != HttpStatus.OK_200)
446+
{
447+
// A failed tunnel attempt, must close the request side.
448+
updateClose(true, CloseState.Event.AFTER_SEND);
449+
}
450+
boolean closed = updateClose(frame.isEndStream(), CloseState.Event.RECEIVED);
451+
notifyHeaders(frame, Callback.from(() ->
452+
{
453+
if (eof)
454+
processData(true);
455+
if (closed)
456+
getSession().removeStream(this);
457+
}, callback));
458+
}
459+
}
447460
}
448461
}
449462

‎jetty-core/jetty-http2/jetty-http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackDecoder.java‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,10 @@
1717
import java.util.function.LongSupplier;
1818

1919
import org.eclipse.jetty.http.HttpField;
20-
import org.eclipse.jetty.http.HttpFields;
2120
import org.eclipse.jetty.http.HttpHeader;
2221
import org.eclipse.jetty.http.HttpTokens;
2322
import org.eclipse.jetty.http.MetaData;
23+
import org.eclipse.jetty.http.PreEncodedHttpField;
2424
import org.eclipse.jetty.http.compression.EncodingException;
2525
import org.eclipse.jetty.http.compression.HuffmanDecoder;
2626
import org.eclipse.jetty.http.compression.NBitIntegerDecoder;
@@ -39,6 +39,7 @@
3939
public class HpackDecoder
4040
{
4141
private static final Logger LOG = LoggerFactory.getLogger(HpackDecoder.class);
42+
private static final HttpField LOWER_CASE_CONTENT_LENGTH_0 = new PreEncodedHttpField(HttpHeader.CONTENT_LENGTH, "content-length", "0");
4243

4344
private final HpackContext _context;
4445
private final MetaDataBuilder _builder;
@@ -123,13 +124,14 @@ public MetaData decode(ByteBuffer buffer) throws HpackException.SessionException
123124
if (entry == null)
124125
throw new HpackException.SessionException("Unknown index %d", index);
125126

127+
HttpField field = entry.getHttpField();
126128
if (entry.isStatic())
127129
{
128130
if (LOG.isDebugEnabled())
129131
LOG.debug("decode IdxStatic {}", entry);
130132
// emit field
131133
emitted = true;
132-
_builder.emit(entry.getHttpField());
134+
_builder.emit(field);
133135

134136
// TODO copy and add to reference set if there is room
135137
// _context.add(entry.getHttpField());
@@ -138,9 +140,18 @@ public MetaData decode(ByteBuffer buffer) throws HpackException.SessionException
138140
{
139141
if (LOG.isDebugEnabled())
140142
LOG.debug("decode Idx {}", entry);
143+
144+
String name = field.getName();
145+
if (!HttpTokens.isLegalH2H3FieldName(name))
146+
_builder.streamException("Illegal header name %s", name);
147+
148+
String value = field.getValue();
149+
if (!HttpTokens.isLegalFieldValue(value))
150+
_builder.streamException("Illegal header value %s", value);
151+
141152
// emit
142153
emitted = true;
143-
_builder.emit(entry.getHttpField());
154+
_builder.emit(field);
144155
}
145156
}
146157
else
@@ -248,7 +259,7 @@ public MetaData decode(ByteBuffer buffer) throws HpackException.SessionException
248259

249260
case CONTENT_LENGTH:
250261
if ("0".equals(value))
251-
field = HttpFields.CONTENT_LENGTH_0;
262+
field = LOWER_CASE_CONTENT_LENGTH_0;
252263
else
253264
field = new HttpField.LongValueHttpField(header, name, value);
254265
break;

‎jetty-core/jetty-http2/jetty-http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/internal/MetaDataBuilder.java‎

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,23 @@ public MetaDataBuilder(int maxHeadersSize)
5151
_maxSize = maxHeadersSize;
5252
}
5353

54+
private void reset()
55+
{
56+
_fields.clear();
57+
_size = 0;
58+
_status = null;
59+
_method = null;
60+
_scheme = null;
61+
_authority = null;
62+
_path = null;
63+
_protocol = null;
64+
_contentLength = -1;
65+
_streamException = null;
66+
_request = false;
67+
_response = false;
68+
_beginNanoTime = Long.MIN_VALUE;
69+
}
70+
5471
/**
5572
* Get the maxSize.
5673
* @return the maxSize
@@ -235,18 +252,14 @@ protected boolean checkPseudoHeader(HttpHeader header, Object value)
235252

236253
public MetaData build() throws HpackException.StreamException
237254
{
238-
if (_streamException != null)
255+
try
239256
{
240-
_streamException.addSuppressed(new Throwable());
241-
throw _streamException;
242-
}
257+
if (_streamException != null)
258+
throw _streamException;
243259

244-
if (_request && _response)
245-
throw new HpackException.StreamException(true, true, "Request and Response headers");
260+
if (_request && _response)
261+
throw new HpackException.StreamException(true, true, "Request and Response headers");
246262

247-
HttpFields.Mutable fields = _fields;
248-
try
249-
{
250263
if (_request)
251264
{
252265
if (_method == null)
@@ -264,7 +277,7 @@ public MetaData build() throws HpackException.StreamException
264277

265278
if (isConnect)
266279
{
267-
return new MetaData.ConnectRequest(nanoTime, _scheme, _authority, _path, fields, _protocol);
280+
return new MetaData.ConnectRequest(nanoTime, _scheme, _authority, _path, _fields, _protocol);
268281
}
269282
else
270283
{
@@ -273,7 +286,7 @@ public MetaData build() throws HpackException.StreamException
273286
_method,
274287
newHttpURI(),
275288
HttpVersion.HTTP_2,
276-
fields,
289+
_fields,
277290
_contentLength,
278291
null);
279292
}
@@ -283,24 +296,14 @@ public MetaData build() throws HpackException.StreamException
283296
{
284297
if (_status == null)
285298
throw new HpackException.StreamException(false, true, "No Status");
286-
return new MetaData.Response(_status, null, HttpVersion.HTTP_2, fields, _contentLength);
299+
return new MetaData.Response(_status, null, HttpVersion.HTTP_2, _fields, _contentLength);
287300
}
288301

289-
return new MetaData(HttpVersion.HTTP_2, fields, _contentLength);
302+
return new MetaData(HttpVersion.HTTP_2, _fields, _contentLength);
290303
}
291304
finally
292305
{
293-
_fields.clear();
294-
_request = false;
295-
_response = false;
296-
_status = null;
297-
_method = null;
298-
_scheme = null;
299-
_authority = null;
300-
_path = null;
301-
_protocol = null;
302-
_size = 0;
303-
_contentLength = -1;
306+
reset();
304307
}
305308
}
306309

0 commit comments

Comments
 (0)