479537 - Server preface sent after client preface reply.

Fixed by anticipating the onPreface() callback on server before
processing the client preface's SETTINGS frame.

6 files changed, 269 insertions, 13 deletions

diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AbstractTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AbstractTest.java
index 62a0608abf..7bc4179f31 100644
--- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AbstractTest.java
+++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/AbstractTest.java
@@ -51,7 +51,7 @@ public class AbstractTest
     protected ServerConnector connector;
     protected String servletPath = "/test";
     protected HTTP2Client client;
-    private Server server;
+    protected Server server;
 
     protected void start(HttpServlet servlet) throws Exception
     {
@@ -71,19 +71,19 @@ public class AbstractTest
 
     protected void start(ServerSessionListener listener) throws Exception
     {
-        prepareServer(new RawHTTP2ServerConnectionFactory(new HttpConfiguration(),listener));
+        prepareServer(new RawHTTP2ServerConnectionFactory(new HttpConfiguration(), listener));
         server.start();
 
         prepareClient();
         client.start();
     }
 
-    private void prepareServer(ConnectionFactory connectionFactory)
+    protected void prepareServer(ConnectionFactory... connectionFactories)
     {
         QueuedThreadPool serverExecutor = new QueuedThreadPool();
         serverExecutor.setName("server");
         server = new Server(serverExecutor);
-        connector = new ServerConnector(server, 1,1, connectionFactory);
+        connector = new ServerConnector(server, 1, 1, connectionFactories);
         server.addConnector(connector);
     }
 
diff --git a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PrefaceTest.java b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PrefaceTest.java
index 31623f8705..9f353ad158 100644
--- a/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PrefaceTest.java
+++ b/jetty-http2/http2-client/src/test/java/org/eclipse/jetty/http2/client/PrefaceTest.java
@@ -18,19 +18,41 @@
 
 package org.eclipse.jetty.http2.client;
 
+import java.net.InetSocketAddress;
+import java.nio.ByteBuffer;
+import java.nio.channels.SocketChannel;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
+import java.util.Queue;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jetty.http.HttpFields;
+import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpVersion;
 import org.eclipse.jetty.http.MetaData;
+import org.eclipse.jetty.http2.ErrorCode;
 import org.eclipse.jetty.http2.api.Session;
 import org.eclipse.jetty.http2.api.Stream;
 import org.eclipse.jetty.http2.api.server.ServerSessionListener;
 import org.eclipse.jetty.http2.frames.HeadersFrame;
+import org.eclipse.jetty.http2.frames.PingFrame;
+import org.eclipse.jetty.http2.frames.PrefaceFrame;
 import org.eclipse.jetty.http2.frames.SettingsFrame;
+import org.eclipse.jetty.http2.generator.Generator;
+import org.eclipse.jetty.http2.parser.Parser;
+import org.eclipse.jetty.http2.server.HTTP2CServerConnectionFactory;
+import org.eclipse.jetty.io.ByteBufferPool;
+import org.eclipse.jetty.io.EndPoint;
+import org.eclipse.jetty.io.MappedByteBufferPool;
+import org.eclipse.jetty.server.Connector;
+import org.eclipse.jetty.server.HttpConfiguration;
+import org.eclipse.jetty.server.HttpConnectionFactory;
+import org.eclipse.jetty.util.ArrayQueue;
 import org.eclipse.jetty.util.Callback;
 import org.eclipse.jetty.util.Promise;
 import org.junit.Assert;
@@ -39,7 +61,7 @@ import org.junit.Test;
 public class PrefaceTest extends AbstractTest
 {
     @Test
-    public void testServerPrefaceBeforeClientPreface() throws Exception
+    public void testServerPrefaceReplySentAfterClientPreface() throws Exception
     {
         start(new ServerSessionListener.Adapter()
         {
@@ -95,4 +117,211 @@ public class PrefaceTest extends AbstractTest
 
         Assert.assertTrue(latch.await(5, TimeUnit.SECONDS));
     }
+
+    @Test
+    public void testClientPrefaceReplySentAfterServerPreface() throws Exception
+    {
+        start(new ServerSessionListener.Adapter()
+        {
+            @Override
+            public Map<Integer, Integer> onPreface(Session session)
+            {
+                Map<Integer, Integer> settings = new HashMap<>();
+                settings.put(SettingsFrame.MAX_CONCURRENT_STREAMS, 128);
+                return settings;
+            }
+
+            @Override
+            public void onPing(Session session, PingFrame frame)
+            {
+                session.close(ErrorCode.NO_ERROR.code, null, Callback.NOOP);
+            }
+        });
+
+        ByteBufferPool byteBufferPool = client.getByteBufferPool();
+        try (SocketChannel socket = SocketChannel.open())
+        {
+            socket.connect(new InetSocketAddress("localhost", connector.getLocalPort()));
+
+            Generator generator = new Generator(byteBufferPool);
+            ByteBufferPool.Lease lease = new ByteBufferPool.Lease(byteBufferPool);
+            generator.control(lease, new PrefaceFrame());
+            Map<Integer, Integer> clientSettings = new HashMap<>();
+            clientSettings.put(SettingsFrame.ENABLE_PUSH, 0);
+            generator.control(lease, new SettingsFrame(clientSettings, false));
+            // The PING frame just to make sure the client stops reading.
+            generator.control(lease, new PingFrame(true));
+
+            List<ByteBuffer> buffers = lease.getByteBuffers();
+            socket.write(buffers.toArray(new ByteBuffer[buffers.size()]));
+
+            Queue<SettingsFrame> settings = new ArrayQueue<>();
+            Parser parser = new Parser(byteBufferPool, new Parser.Listener.Adapter()
+            {
+                @Override
+                public void onSettings(SettingsFrame frame)
+                {
+                    settings.offer(frame);
+                }
+            }, 4096, 8192);
+
+            ByteBuffer buffer = byteBufferPool.acquire(1024, true);
+            while (true)
+            {
+                int read = socket.read(buffer);
+                buffer.flip();
+                if (read < 0)
+                    break;
+                parser.parse(buffer);
+                buffer.clear();
+            }
+
+            Assert.assertEquals(2, settings.size());
+            SettingsFrame frame1 = settings.poll();
+            Assert.assertFalse(frame1.isReply());
+            SettingsFrame frame2 = settings.poll();
+            Assert.assertTrue(frame2.isReply());
+        }
+    }
+
+    @Test
+    public void testOnPrefaceNotifiedForStandardUpgrade() throws Exception
+    {
+        Integer maxConcurrentStreams = 128;
+        AtomicReference<CountDownLatch> serverPrefaceLatch = new AtomicReference<>(new CountDownLatch(1));
+        AtomicReference<CountDownLatch> serverSettingsLatch = new AtomicReference<>(new CountDownLatch(1));
+        HttpConfiguration config = new HttpConfiguration();
+        prepareServer(new HttpConnectionFactory(config), new HTTP2CServerConnectionFactory(config)
+        {
+            @Override
+            protected ServerSessionListener newSessionListener(Connector connector, EndPoint endPoint)
+            {
+                return new ServerSessionListener.Adapter()
+                {
+                    @Override
+                    public Map<Integer, Integer> onPreface(Session session)
+                    {
+                        Map<Integer, Integer> serverSettings = new HashMap<>();
+                        serverSettings.put(SettingsFrame.MAX_CONCURRENT_STREAMS, maxConcurrentStreams);
+                        serverPrefaceLatch.get().countDown();
+                        return serverSettings;
+                    }
+
+                    @Override
+                    public void onSettings(Session session, SettingsFrame frame)
+                    {
+                        serverSettingsLatch.get().countDown();
+                    }
+
+                    @Override
+                    public Stream.Listener onNewStream(Stream stream, HeadersFrame frame)
+                    {
+                        MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, HttpStatus.OK_200, new HttpFields());
+                        stream.headers(new HeadersFrame(stream.getId(), response, null, true), Callback.NOOP);
+                        return null;
+                    }
+                };
+            }
+        });
+        server.start();
+
+        ByteBufferPool byteBufferPool = new MappedByteBufferPool();
+        try (SocketChannel socket = SocketChannel.open())
+        {
+            socket.connect(new InetSocketAddress("localhost", connector.getLocalPort()));
+
+            String upgradeRequest = "" +
+                    "GET /one HTTP/1.1\r\n" +
+                    "Host: localhost\r\n" +
+                    "Connection: Upgrade, HTTP2-Settings\r\n" +
+                    "Upgrade: h2c\r\n" +
+                    "HTTP2-Settings: \r\n" +
+                    "\r\n";
+            ByteBuffer upgradeBuffer = ByteBuffer.wrap(upgradeRequest.getBytes(StandardCharsets.ISO_8859_1));
+            socket.write(upgradeBuffer);
+
+            // Make sure onPreface() is called on server.
+            Assert.assertTrue(serverPrefaceLatch.get().await(5, TimeUnit.SECONDS));
+            Assert.assertTrue(serverSettingsLatch.get().await(5, TimeUnit.SECONDS));
+
+            // The 101 response is the reply to the client preface SETTINGS frame.
+            ByteBuffer buffer = byteBufferPool.acquire(1024, true);
+            http1: while (true)
+            {
+                buffer.clear();
+                int read = socket.read(buffer);
+                buffer.flip();
+                if (read < 0)
+                    Assert.fail();
+
+                int crlfs = 0;
+                while (buffer.hasRemaining())
+                {
+                    byte b = buffer.get();
+                    if (b == '\r' || b == '\n')
+                        ++crlfs;
+                    else
+                        crlfs = 0;
+                    if (crlfs == 4)
+                        break http1;
+                }
+            }
+
+            // Reset the latches on server.
+            serverPrefaceLatch.set(new CountDownLatch(1));
+            serverSettingsLatch.set(new CountDownLatch(1));
+
+            // After the 101, the client must send the connection preface.
+            Generator generator = new Generator(byteBufferPool);
+            ByteBufferPool.Lease lease = new ByteBufferPool.Lease(byteBufferPool);
+            generator.control(lease, new PrefaceFrame());
+            Map<Integer, Integer> clientSettings = new HashMap<>();
+            clientSettings.put(SettingsFrame.ENABLE_PUSH, 1);
+            generator.control(lease, new SettingsFrame(clientSettings, false));
+            List<ByteBuffer> buffers = lease.getByteBuffers();
+            socket.write(buffers.toArray(new ByteBuffer[buffers.size()]));
+
+            // However, we should not call onPreface() again.
+            Assert.assertFalse(serverPrefaceLatch.get().await(1, TimeUnit.SECONDS));
+            // Although we should notify of the SETTINGS frame.
+            Assert.assertTrue(serverSettingsLatch.get().await(5, TimeUnit.SECONDS));
+
+            CountDownLatch clientSettingsLatch = new CountDownLatch(1);
+            AtomicBoolean responded = new AtomicBoolean();
+            Parser parser = new Parser(byteBufferPool, new Parser.Listener.Adapter()
+            {
+                @Override
+                public void onSettings(SettingsFrame frame)
+                {
+                    if (frame.isReply())
+                        return;
+                    Assert.assertEquals(maxConcurrentStreams, frame.getSettings().get(SettingsFrame.MAX_CONCURRENT_STREAMS));
+                    clientSettingsLatch.countDown();
+                }
+
+                @Override
+                public void onHeaders(HeadersFrame frame)
+                {
+                    if (frame.isEndStream())
+                        responded.set(true);
+                }
+            }, 4096, 8192);
+
+            // HTTP/2 parsing.
+            while (true)
+            {
+                parser.parse(buffer);
+                if (responded.get())
+                    break;
+
+                buffer.clear();
+                int read = socket.read(buffer);
+                buffer.flip();
+                if (read < 0)
+                    Assert.fail();
+            }
+
+            Assert.assertTrue(clientSettingsLatch.await(5, TimeUnit.SECONDS));
+        }
+    }
 }
diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java
index b8ffbcb56b..cfc27e43c0 100644
--- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java
+++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/Parser.java
@@ -197,6 +197,11 @@ public class Parser
         return headerParser.getFrameType();
     }
 
+    protected boolean hasFlag(int bit)
+    {
+        return headerParser.hasFlag(bit);
+    }
+
     protected void notifyConnectionFailure(int error, String reason)
     {
         try
diff --git a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/ServerParser.java b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/ServerParser.java
index f5d1718de7..c5843481ea 100644
--- a/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/ServerParser.java
+++ b/jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/parser/ServerParser.java
@@ -21,6 +21,7 @@ package org.eclipse.jetty.http2.parser;
 import java.nio.ByteBuffer;
 
 import org.eclipse.jetty.http2.ErrorCode;
+import org.eclipse.jetty.http2.Flags;
 import org.eclipse.jetty.http2.frames.FrameType;
 import org.eclipse.jetty.io.ByteBufferPool;
 import org.eclipse.jetty.util.BufferUtil;
@@ -34,6 +35,7 @@ public class ServerParser extends Parser
     private final Listener listener;
     private final PrefaceParser prefaceParser;
     private State state = State.PREFACE;
+    private boolean notifyPreface = true;
 
     public ServerParser(ByteBufferPool byteBufferPool, Listener listener, int maxDynamicTableSize, int maxHeaderSize)
     {
@@ -61,6 +63,16 @@ public class ServerParser extends Parser
         prefaceParser.directUpgrade();
     }
 
+    /**
+     * <p>The standard HTTP/1.1 upgrade path.</p>
+     */
+    public void standardUpgrade()
+    {
+        if (state != State.PREFACE)
+            throw new IllegalStateException();
+        notifyPreface = false;
+    }
+
     @Override
     public void parse(ByteBuffer buffer)
     {
@@ -77,6 +89,8 @@ public class ServerParser extends Parser
                     {
                         if (!prefaceParser.parse(buffer))
                             return;
+                        if (notifyPreface)
+                            onPreface();
                         state = State.SETTINGS;
                         break;
                     }
@@ -84,7 +98,7 @@ public class ServerParser extends Parser
                     {
                         if (!parseHeader(buffer))
                             return;
-                        if (getFrameType() != FrameType.SETTINGS.getType())
+                        if (getFrameType() != FrameType.SETTINGS.getType() || hasFlag(Flags.ACK))
                         {
                             BufferUtil.clear(buffer);
                             notifyConnectionFailure(ErrorCode.PROTOCOL_ERROR.code, "invalid_preface");
@@ -92,7 +106,6 @@ public class ServerParser extends Parser
                         }
                         if (!parseBody(buffer))
                             return;
-                        onPreface();
                         state = State.FRAMES;
                         break;
                     }
diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java
index 07ffd987ad..68dd541bc4 100644
--- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java
+++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java
@@ -19,6 +19,8 @@
 package org.eclipse.jetty.http2.server;
 
 import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Queue;
 import java.util.concurrent.Executor;
 
@@ -33,7 +35,9 @@ import org.eclipse.jetty.http2.ISession;
 import org.eclipse.jetty.http2.IStream;
 import org.eclipse.jetty.http2.api.server.ServerSessionListener;
 import org.eclipse.jetty.http2.frames.DataFrame;
+import org.eclipse.jetty.http2.frames.Frame;
 import org.eclipse.jetty.http2.frames.HeadersFrame;
+import org.eclipse.jetty.http2.frames.PrefaceFrame;
 import org.eclipse.jetty.http2.frames.SettingsFrame;
 import org.eclipse.jetty.http2.parser.ServerParser;
 import org.eclipse.jetty.http2.parser.SettingsBodyParser;
@@ -53,7 +57,7 @@ public class HTTP2ServerConnection extends HTTP2Connection implements Connection
     private final Queue<HttpChannelOverHTTP2> channels = new ConcurrentArrayQueue<>();
     private final ServerSessionListener listener;
     private final HttpConfiguration httpConfig;
-    private HeadersFrame upgradeRequest;
+    private final List<Frame> upgradeFrames = new ArrayList<>();
 
     public HTTP2ServerConnection(ByteBufferPool byteBufferPool, Executor executor, EndPoint endPoint, HttpConfiguration httpConfig, ServerParser parser, ISession session, int inputBufferSize, ServerSessionListener listener)
     {
@@ -79,10 +83,10 @@ public class HTTP2ServerConnection extends HTTP2Connection implements Connection
     @Override
     public void onOpen()
     {
-        super.onOpen();
         notifyAccept(getSession());
-        if (upgradeRequest != null)
-            getSession().onFrame(upgradeRequest);
+        for (Frame frame : upgradeFrames)
+            getSession().onFrame(frame);
+        super.onOpen();
     }
 
     private void notifyAccept(ISession session)
@@ -172,10 +176,12 @@ public class HTTP2ServerConnection extends HTTP2Connection implements Connection
                 throw new BadMessageException();
             }
 
-            getSession().onFrame(settingsFrame);
+            getParser().standardUpgrade();
 
+            upgradeFrames.add(new PrefaceFrame());
+            upgradeFrames.add(settingsFrame);
             // Remember the request to send a response from onOpen().
-            upgradeRequest = new HeadersFrame(1, request, null, true);
+            upgradeFrames.add(new HeadersFrame(1, request, null, true));
         }
         return true;
     }
diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java
index 7d53e206af..59fede564c 100644
--- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java
+++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerSession.java
@@ -112,6 +112,9 @@ public class HTTP2ServerSession extends HTTP2Session implements ServerParser.Lis
     {
         switch (frame.getType())
         {
+            case PREFACE:
+                onPreface();
+                break;
             case SETTINGS:
                 // SPEC: the required reply to this SETTINGS frame is the 101 response.
                 onSettings((SettingsFrame)frame, false);